Skip to content

New channel modal toggle tabs#2643

Merged
marcellamaki merged 5 commits into
learningequality:developfrom
marcellamaki:new-channel-modal-toggle-tabs
Dec 10, 2020
Merged

New channel modal toggle tabs#2643
marcellamaki merged 5 commits into
learningequality:developfrom
marcellamaki:new-channel-modal-toggle-tabs

Conversation

@marcellamaki
Copy link
Copy Markdown
Member

Description

Updates routing to use a /:tab value that can be either 'edit' or 'share' rather than the query sharing=true in the share and detail modal. Also makes sure the tab indicator aligns with 'share' correctly on open. Replaces PR2547.

Issue Addressed (if applicable)

Addresses #2004

Before/After Screenshots (if applicable)

toggle-tabs

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

Copy link
Copy Markdown
Member

@MisRob MisRob left a comment

Choose a reason for hiding this comment

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

Thank you, @marcellamaki, all is working as expected.

Before merging, could you address the following edge-case? When I insert an URL directly to address bar containing unsupported tab value (example: /en/channels/#/66a31fdbdd745c7c9e499206f3c4cced/abc?last=CHANNELS_EDITABLE) then I get:

Screenshot from 2020-12-09 11-37-18

Maybe we could just redirect to /edit by default when the value provided is not supported (this is how it worked till now automatically due to using edit as the default value of sharing query parameter)?

tab: value,
},
})
.catch(() => {});
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.

What's the purpose of this catch?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I don't remember off the top of my head, but I know there was a purpose, as I remember researching it. I will see if I can look it up again and I will follow up with you!

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.

I am afraid that this might result in losing information or swallowing errors that need to be handled. It's okay if there is a reason - it would be nice to add the information to the comment.

expect(wrapper.find('[data-test="share-content"]').isVisible()).toBe(true);
expect(wrapper.find('[data-test="edit-content"]').isVisible()).toBe(false);
});
it('when the current tab is edit, the edit content should display in the modal', async () => {
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.

👍

Sign up for free to 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.

2 participants