Skip to content

Flatten routes 2 - channel list - channel edit modal and channel details modal - #2024

Merged
jayoshih merged 7 commits into
learningequality:developfrom
MisRob:router-cleanup-channel-list
Aug 10, 2020
Merged

Flatten routes 2 - channel list - channel edit modal and channel details modal#2024
jayoshih merged 7 commits into
learningequality:developfrom
MisRob:router-cleanup-channel-list

Conversation

@MisRob

@MisRobMisRob commented Aug 4, 2020

Copy link
Copy Markdown
Member

Description

Another PR to flatten routes + related refactoring and cleanup.
I added another commit to shorten route paths (seems that there's no need to have channels two times in URL)

Collections will follow soon.

Steps to Test

Navigate to channel information and channel edit modals from channel lists and public catalog (these modals are also rendered when we navigate to them from channelEdit app though I haven't updated its routes in this regard yet, will follow in upcoming PRs). Check that back navigation works properly. Try to reload pages.

@MisRob
MisRob requested a review from rtibblesAugust 4, 2020 08:27
@MisRobMisRob added this to the Vue Refactor milestone Aug 4, 2020
@MisRobMisRob changed the title Flatten routes - channel list - channel edit modal and channel details modalFlatten routes 2 - channel list - channel edit modal and channel details modalAug 4, 2020
@codecov

codecovBot commented Aug 4, 2020

Copy link
Copy Markdown

Codecov Report

Merging #2024 into develop will increase coverage by 0.01%.
The diff coverage is n/a.

Impacted file tree graph

@@ Coverage Diff @@## develop #2024 +/- ##
===========================================
+ Coverage 79.46% 79.47% +0.01% 
===========================================
Files 276 281 +5 Lines 13275 13318 +43 ===========================================
+ Hits 10549 10585 +36 - Misses 2726 2733 +7 
Impacted FilesCoverage Δ
contentcuration/search/viewsets/contentnode.py58.51% <0.00%> (-3.56%)⬇️
...ontentcuration/contentcuration/viewsets/channel.py78.70% <0.00%> (-0.20%)⬇️
contentcuration/contentcuration/utils/publish.py77.03% <0.00%> (-0.08%)⬇️
...uration/kolibri_content/migrations/0001_initial.py100.00% <0.00%> (ø)
...entcuration/migrations/0112_merge_20200513_0049.py
...entcuration/migrations/0113_merge_20200513_2256.py
...ibri_content/migrations/0007_auto_20200613_0050.py100.00% <0.00%> (ø)
...migrations/0115_index_contentnode_node_id_field.py100.00% <0.00%> (ø)
...entcuration/migrations/0114_merge_20200806_1746.py100.00% <0.00%> (ø)
...content/migrations/0008_channelmetadata_tagline.py100.00% <0.00%> (ø)
... and 6 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 579300c...151a14f. Read the comment docs.

@MisRob
MisRobforce-pushed the router-cleanup-channel-list branch from 70a0cd1 to d7c0c40CompareAugust 4, 2020 10:29

@rtibblesrtibbles left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A couple of questions.

@indirectlylitindirectlylit left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor note that as the number of constants and mappings between constants grows, maintenance can get a bit challenging.

Of course using constants is usually better than raw strings, but there are also some cases where this can be taken too far, and some other kind of abstraction is necessary.

As noted below, it's also important to clearly define what constants are used for, and use them only for that purpose. Otherwise we unintentionally couple things that have no need to be coupled.

Comment threadcontentcuration/contentcuration/frontend/channelList/router.js Outdated

@jayoshihjayoshih left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just one change on maintaining query params, but looks good otherwise!

@MisRobMisRob mentioned this pull request Aug 6, 2020
@MisRob
MisRob requested a review from jayoshihAugust 6, 2020 11:18

@jayoshihjayoshih left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice!!!

@jayoshih
jayoshih merged commit 9100bdd into learningequality:developAug 10, 2020
@MisRob
MisRob deleted the router-cleanup-channel-list branch November 3, 2020 06:18
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@MisRob@rtibbles@indirectlylit@jayoshih