openedx / modular-learning

3 stars 1 forks source link

[Tagging] UX Refinements from Sandbox Round 1 #157

Closed bradenmacdonald closed 8 months ago

bradenmacdonald commented 9 months ago

Changes we've collected from initial testing:

On edx-platform

  1. Comment: "Please change the order of the items in the unit menu to:"
    • Configure
    • Manage tags
    • Copy to clipboard
    • Duplicate
    • Delete
  2. Comment: "Please change the order of the items in the component menu to:"
    • Manage Access
    • Move
    • Manage tags
    • Copy to Clipboard
    • Duplicate
    • Delete
  3. Comment: "There is a strange 'jump' when I first expand the "Tags" sidebar component. The area containing the "Tags" heading, tag count, and up/down arrow seems to shift down a few pixels when it is first clicked"
  4. Comment: This section also shows a dotted outline after it is selected (see screenshot below): Screenshot 2023-11-28 at 13 49 39
    • Note: it is important that this element is keyboard focusable and when it is focused using they keyboard, some visual outline must appear. We want to preserve that but also to not show that outline when it's clicked using the mouse. This can be achieved using :focus-visible.
  5. Comment: "If a tag cannot be expanded (i.e. leaf tags), it should not have a hover state (in the screenshot below, "flat taxonomy tag 2949" should be black, not blue):" Screenshot 2023-11-28 at 14 00 00
  6. Slack: Refresh the count of tags on the unit/outline page(s) after the user makes edits in the tag drawer
    • Moved to #167
  7. Change the default URL for the "Taxonomies" tab to go to /taxonomies/ directly, not /taxonomy-list/

Within the course-authoring MFE

  1. Comment: On the taxonomies list, display the full name of taxonomies in a tooltip if it's longer than what's displayed
  2. Comment: On the taxonomy detail page, please change the title of the "Value" column to "Tag name" https://github.com/openedx/modular-learning/issues/138
  3. Comment: On taxonomy detail page, remove the "child tags" column and put it in parentheses instead
  4. Comment - several minor issues. See link.
  5. Slack: Make your browser window short (not too tall). From the Taxonomy List page, scroll down and click on "WGU" taxonomy. The taxonomy detail page loads, but it is already scrolled down. It should load with scroll=0. Figure out why react-router isn't properly resetting the scroll to 0 when clicking a link, as it should do for every page transition.
  6. Slack: On this page, clicking "Manage Tags" for a component (not the unit) that has no tags, then clicking Add Tags at the top makes the list of tags appear offscreen.
    • Screenshot 2023-12-01 at 9 57 28 AM
    • then the tag list gets cut off at the top: Screenshot 2023-12-01 at 9 57 44 AM

To consider for the future (out of scope for now)

ChrisChV commented 9 months ago

Hi @ali-hugo about this:

On taxonomy detail page, remove the "child tags" column and put it in parentheses instead

This is the result:

image

It's the same of Figma, but on this comment you mention that it will be a gray number. Can you confirm what the expected design is?

bradenmacdonald commented 9 months ago

@ChrisChV You probably noticed this, but I've been adding a few additional little issues to this ticket as they come in. If there are too many to address within one sprint/task, feel free to leave some of them for next sprint.

ali-hugo commented 9 months ago

@ChrisChV

It's the same of Figma, but on this https://github.com/openedx/modular-learning/issues/112#issuecomment-1827412152 you mention that it will be a gray number. Can you confirm what the expected design is?

As you have it is correct (sorry, I don't know why I thought the number was gray). Thanks for making that update. :)

bradenmacdonald commented 9 months ago

@ali-hugo Probably because the numbers are currently grey for the child/grandchild tag counts on the sandbox.

ali-hugo commented 9 months ago

@bradenmacdonald

Probably because the numbers are currently grey for the child/grandchild tag counts on the sandbox.

Yip, that's it! They're actually supposed to be dark grey.

@ChrisChV I see that the colour that's currently being used for the child/grandchild numbers fails an accessibility check (when it appears on the grey rows of the table). Could you change all the numbers (the ones next to the child/grandchild tags, and the ones next to the parent tags) to #454545 please?

Screenshot 2023-12-05 at 11 11 23
ChrisChV commented 9 months ago
  1. Slack: On this page, clicking "Manage Tags" for a component (not the unit) that has no tags, then clicking Add Tags at the top makes the list of tags appear offscreen.

@yusuf-musleh This bug has been introduced in your PR https://github.com/openedx/frontend-app-course-authoring/pull/704 Only happens in your branch and in the sandbox that is updated also with your changes. If it's a quick change, can you fix it? or maybe it's better to fix it in another round of refinements? CC @bradenmacdonald

ChrisChV commented 9 months ago

I see that the colour that's currently being used for the child/grandchild numbers fails an accessibility check (when it appears on the grey rows of the table). Could you change all the numbers (the ones next to the child/grandchild tags, and the ones next to the parent tags) to #454545 please?

@ali-hugo Sure :+1:

ali-hugo commented 9 months ago

@rpenido @ChrisChV I'm CC-ing you both as I'm not sure who would work on the changes I've requested below. Please let me know if I should move this request elsewhere:

When testing on the sandbox yesterday, I noticed there was something missing from the importing/re-importing flow in the design for the taxonomy list page: there is no indication to the user that the taxonomy is being imported/re-imported.

Could we add a spinner to the card of the taxonomy that is being imported/re-imported? Like so (I thought it might be neater to place the icon to the left of the title since longer titles sometimes have ellipses to the right):

Screenshot 2023-12-06 at 08 44 15

Could we also add a toast to the bottom left of the page?:

Screenshot 2023-12-06 at 08 44 25

I think that would help to keep the user updated on what's happening.

CC @bradenmacdonald

yusuf-musleh commented 9 months ago
  1. Slack: On this page, clicking "Manage Tags" for a component (not the unit) that has no tags, then clicking Add Tags at the top makes the list of tags appear offscreen.

@yusuf-musleh This bug has been introduced in your PR openedx/frontend-app-course-authoring#704 Only happens in your branch and in the sandbox that is updated also with your changes. If it's a quick change, can you fix it? or maybe it's better to fix it in another round of refinements? CC @bradenmacdonald

@ChrisChV Thanks for pointing it out. I managed to reproduce it as well. The ModalPopup in Paragon has been a bit flaky in the placement, I've been having a hard time getting it to work properly, especially in these edge cases. Adding the hasArrow prop fixes this, however brings back another 2 issues mentioned before:

  1. The popup jumping around when adding/removing tags. This can be fixed when we change the implementation to add/remove the actual tags when the popup is closed vs when on the fly when checking/unchecking tags
  2. The arrow's placement is off. This is an issue in Paragon as well, but we can set the arrow to be visibility: hidden for it to now show for now, and create an issue in Paragon repo so a fix can be worked on later.

I think I can include a fix for it as part of my PR, essentially including the hasArrow prop, hiding the arrow, and ~changing the implementation to update the tags when the popup modal is closed rather than on the fly.~

Update: On second thought, the changes required for updating the tags when the popup modal closes would be too involved to include in this PR, so I will leave it to a separate followup PR. But I've implemented the remaining changes and it seems to fix the issue.

rpenido commented 9 months ago

@rpenido @ChrisChV I'm CC-ing you both as I'm not sure who would work on the changes I've requested below. Please let me know if I should move this request elsewhere:

When testing on the sandbox yesterday, I noticed there was something missing from the importing/re-importing flow in the design for the taxonomy list page: there is no indication to the user that the taxonomy is being imported/re-imported.

Could we add a spinner to the card of the taxonomy that is being imported/re-imported? Like so (I thought it might be neater to place the icon to the left of the title since longer titles sometimes have ellipses to the right):

Screenshot 2023-12-06 at 08 44 15

Could we also add a toast to the bottom left of the page?:

Screenshot 2023-12-06 at 08 44 25

I think that would help to keep the user updated on what's happening.

Hi @ali-hugo! For now, we are running the import tag synchronously, so as soon as the dialog is closed, the tag will be imported. I will add a Toast as part of https://github.com/openedx/modular-learning/issues/126.

import_tags

I added LoadingButtons to the operations that do some work on the server.

PS: The design is a bit different than the wireframes. I will discuss it with you in the issue https://github.com/openedx/modular-learning/issues/126 soon!

ChrisChV commented 9 months ago
  1. Slack: Refresh the count of tags on the unit/outline page(s) after the user makes edits in the tag drawer only if easy to do. Do not spend a lot of time on this now, as it will basically happen automatically in the new MFE version of these pages, thanks to using react-query.

@bradenmacdonald I can't fix this issue because https://github.com/openedx/frontend-app-course-authoring/pull/704 is still open. FYI @yusuf-musleh

yusuf-musleh commented 9 months ago
  1. Slack: Refresh the count of tags on the unit/outline page(s) after the user makes edits in the tag drawer only if easy to do. Do not spend a lot of time on this now, as it will basically happen automatically in the new MFE version of these pages, thanks to using react-query.

@bradenmacdonald I can't fix this issue because openedx/frontend-app-course-authoring#704 is still open. FYI @yusuf-musleh

@ChrisChV Should be merged soon, awaiting final review from @xitij2000.

bradenmacdonald commented 9 months ago

@ChrisChV That's OK, if one or two things are blocked or not possible to fit into this sprint, we can move them to a future ticket. As long as we don't lose track of them.

ChrisChV commented 9 months ago

@ChrisChV That's OK, if one or two things are blocked or not possible to fit into this sprint, we can move them to a future ticket. As long as we don't lose track of them.

Is there another ticket to add these remaining issues?

ChrisChV commented 9 months ago

@bradenmacdonald ^

bradenmacdonald commented 9 months ago

@ChrisChV Please add them here: https://github.com/openedx/modular-learning/issues/167