Skip to content

Channel modal toggle routing - #2547

Closed
marcellamaki wants to merge 194 commits into
learningequality:developfrom
marcellamaki:channel-modal-toggle-routing
Closed

Channel modal toggle routing#2547
marcellamaki wants to merge 194 commits into
learningequality:developfrom
marcellamaki:channel-modal-toggle-routing

Conversation

@marcellamaki

@marcellamakimarcellamaki commented Nov 19, 2020

Copy link
Copy Markdown
Member

Please remove any unused sections

Description

Updates routing to use params rather than the query sharing=true in the share and detail modal, and make sure the tab indicator aligns with "share" correctly on open.

Issue Addressed (if applicable)

Addresses #2004

Before/After Screenshots (if applicable)

new-modal

Steps to Test

Option 1:

  • Click into channel
  • Access modal from "Edit Channel Details" or "Share Channel"

Option 2:

  • Access modal from the channel list view through the options drop-down menu

Comments

This is a WIP and while it does seem to be working, I think I will need to make some modifications with my strings, as there are some place where I am using specific values for the tab param (i.e. "Share" and "Edit). I am considering using values that are already available, so that strings don't need to be added - perhaps the same as the tab names (details and sharing)? I would appreciate any recommendations here or elsewhere in the changes.

@codecov

codecovBot commented Nov 19, 2020

Copy link
Copy Markdown

Codecov Report

Merging #2547 (e600e08) into develop (90cc94e) will increase coverage by 0.00%.
The diff coverage is 25.00%.

Impacted file tree graph

@@ Coverage Diff @@## develop #2547 +/- ##
========================================
Coverage 84.65% 84.66% ========================================
Files 296 296 Lines 15011 15007 -4 ========================================
- Hits 12708 12705 -3 + Misses 2303 2302 -1 
Impacted FilesCoverage Δ
...tentcuration/contentcuration/utils/google_drive.py24.32% <25.00%> (-4.95%)⬇️

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 90cc94e...baf8fcb. Read the comment docs.

@MisRobMisRob 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.

Thank you, @marcellamaki.

The code looks good. I manually tested the page, and everything is working very well except for one issue:

  1. Click "Edit channel details" (that takes you to "Details" tab of the channel modal page)
  2. Navigate to "Sharing" tab
  3. Close the modal
  4. Click "Edit channel details" again

"Sharing" tab is open

Expected
"Details" tab is open

This is happening from both channel list and channel editor pages.


I think I will need to make some modifications with my strings, as there are some place where I am using specific values for the tab param (i.e. "Share" and "Edit). I am considering using values that are already available, so that strings don't need to be added - perhaps the same as the tab names (details and sharing)? I would appreciate any recommendations here or elsewhere in the changes.

I believe that we only need to be concerned about adding new user-facing strings for translations, and those that you've added don't seem so, is that right?

nucleogenesisand others added 27 commits December 4, 2020 12:21
Better error handling in change event handlers
…nnelsetlist-by-user
Filter channelsetlist by user
…pen-in-order
Make sure changes are merged in order
…pen-in-order
Correctly order changes when looking for the last change
…ent-item-ordering-arrows
Fix assessment item reordering
…_paginate_admin_channels
annotate after paginate Admin channels and users
Being more semantic when annotate after paginate is used
…pen-in-order
Ensure changes are merged in order, log an error if not.
…-in-window.urls
Ensure window.Urls has correct language code & browser lang is respected
…refactor to use props rather than refer directly to route
@marcellamakimarcellamaki mentioned this pull request Dec 8, 2020
3 tasks
@rtibbles

Copy link
Copy Markdown
Member

Superseded by #2643

@rtibblesrtibbles closed this Dec 8, 2020
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.

12 participants

@marcellamaki@rtibbles@MisRob@jayoshih@bjester@kollivier@micahscopes@sairina@nucleogenesis@jonboiser@aronasorman@jredrejo