Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

change: [M3-7540] - Improve tags experience #10122

Merged

Conversation

hkhalil-akamai
Copy link
Contributor

@hkhalil-akamai hkhalil-akamai commented Jan 29, 2024

Description 📝

Update: it appears that the React 18 upgrade resolved the underlying bug! 🪄 Keeping this PR open without the workaround since it includes some improvements to the tag UX.

Fixes a bug in the TagCell component's overflow detection logic that caused it to sometimes render incorrectly. This bug only appeared in production builds of Cloud Manager and was caused by a parent Grid component briefly rendering as display: none, breaking the overflow detection in TagCell.

To fix this issue, this PR introduces a somewhat convoluted workaround: upon mount, TagCell locates its nearest ancestor in the DOM with display: none and then attaches a MutationObserver (via the newly introduced useMutationObserver hook) to that element, re-computing overflow when it is finally displayed.

This PR also includes improvements to the display of the TagCell component and AddTag input on small screens.

Changes 🔄

  • Fix overflow detection using useMutationObserver on parent element
  • Increase width of AddTag input on small screens
  • Always render "Add a tag" and ellipses buttons to the right of tags
  • Upgrade deprecated Select component to Autocomplete (this also fixes a momentary flicker when adding a tag)
  • Replace TagsPanel component with TagCell (these components shared a lot of logic)

Preview 📷

Before After
Not detecting overflow Screenshot 2024-01-29 at 4 55 00 PM Correctly detecting overflow Screenshot 2024-01-29 at 4 55 16 PM
AddTag unusably small on small displays: Screenshot 2024-01-29 at 4 56 21 PM AddTag takes up entire width: Screenshot 2024-01-29 at 4 56 26 PM
"Add a tag" and ellipses buttons appear to the left of existing tags on small displays: Screenshot 2024-01-29 at 4 55 56 PM Buttons now appear to the right of new tags, consistent with the "shadow" effect: Screenshot 2024-01-29 at 4 56 11 PM

How to test 🧪

Prerequisites

As far as I am aware, this bug is only reproducible in production builds of Cloud Manager. For reproducing or testing, use either:

Reproduction steps

(For overflow detection bug)

  • Add several tags to a Linode until they overflow in the Linode details footer
  • Navigate back to the Linodes landing page (/linodes)
  • Refresh the page
  • Return to the details page for the Linode
  • Observe that overflow detection fails, causing tags to clip instead of displaying the "shadow" effect and ellipses button

(For style changes on small screens)

  • In the details page for a Linode with overflowing tags, reduce the window size until the mobile layout is activated
  • Observe that the "Add a tag" and ellipses buttons appear to the left of the tags, inconsistent with how they're displayed on larger screens and unintuitive with the "shadow" UX metaphor indicating an overflow
  • Click on the the "Add a tag" button
  • Observe the tag input field is unusably small, with placeholder and tag labels clipped

As an Author I have considered 🤔

  • 👀 Doing a self review
  • ❔ Our contribution guidelines
  • 🤏 Splitting feature into small PRs
  • ➕ Adding a changeset
  • 🧪 Providing/Improving test coverage
  • 🔐 Removing all sensitive information from the code and PR description
  • 🚩 Using a feature flag to protect the release
  • 👣 Providing comprehensive reproduction steps
  • 📑 Providing or updating our documentation
  • 🕛 Scheduling a pair reviewing session
  • 📱 Providing mobile support
  • ♿ Providing accessibility support

packages/manager/src/components/TagCell/TagCell.tsx Outdated Show resolved Hide resolved
packages/manager/src/components/TagCell/TagCell.tsx Outdated Show resolved Hide resolved
'& > button': {
marginRight: theme.spacing(0.5),
},
flexDirection: 'row-reverse',
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure why these styles were included before specifically for small screens, but it is arguably more intuitive to render the ellipses button next to the shadow effect when tags are overflowing, and the "Add a tag button" at the very right, in a LTR context.

Before After
Screenshot 2024-01-29 at 4 55 56 PM Screenshot 2024-01-29 at 4 56 11 PM

Copy link
Contributor

Choose a reason for hiding this comment

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

Makes sense to me - good change

@hkhalil-akamai hkhalil-akamai added Bug Fixes for regressions or bugs UX/UI Changes for UI/UX to review Ready for Review and removed Work in Progress labels Jan 29, 2024
@hkhalil-akamai hkhalil-akamai marked this pull request as ready for review January 29, 2024 22:38
@hkhalil-akamai hkhalil-akamai requested a review from a team as a code owner January 29, 2024 22:38
@hkhalil-akamai hkhalil-akamai requested review from bnussman-akamai and hana-akamai and removed request for a team January 29, 2024 22:38
},
flexDirection: 'row-reverse',
},
width: '100%',
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Before After
Screenshot 2024-01-29 at 4 56 21 PM Screenshot 2024-01-29 at 4 56 26 PM

@hkhalil-akamai hkhalil-akamai changed the title fix: [M3-7540] - Fix tag overflow detection fix: [M3-7540] - Fix tag overflow detection and improve tags experience Feb 27, 2024
@hkhalil-akamai hkhalil-akamai changed the title fix: [M3-7540] - Fix tag overflow detection and improve tags experience fix: [M3-7540] - Improve tags experience Feb 28, 2024
@hkhalil-akamai
Copy link
Contributor Author

  • In the tag drawer, the Autocomplete is opening randomly after I add a tag

Ah, good catch. Seems like this was caused by differences in how Chrome vs Safari trigger onblur.

  • Somehow I'm able to make a tag look more transparent than the others

This is a new indication for a tag that is in progress of being deleted. Since multiple tags could be deleted at the same time, sometimes this would cause a race condition leading to inconsistent states. Fixed with a new useAtomic hook.

@hkhalil-akamai
Copy link
Contributor Author

@abailly-akamai Thanks for the feedback. Took it a step further and completely removed TagsPanel in favor of TagCell, since I noticed most of the logic was being duplicated (aa3c9).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Introducing this as a new hook as I believe it could be used wherever we asynchronously update a remote resource based on user input.

Copy link
Contributor

@abailly-akamai abailly-akamai left a comment

Choose a reason for hiding this comment

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

Thanks for addressing the UI issues @hkhalil-akamai - the improved UI looks great!

I've left comments about the nitty gritty of your PR, specifically around the new hook you introduced and extra complexity. Happy to pair up to discuss those further

Copy link
Member

@bnussman-akamai bnussman-akamai left a comment

Choose a reason for hiding this comment

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

Looking good. Can we add a loading state to this action?

Screen.Recording.2024-04-01.at.10.01.52.AM.mov

Copy link
Member

@bnussman-akamai bnussman-akamai left a comment

Choose a reason for hiding this comment

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

Thanks for sticking with this PR for so long! Overall, I think this makes some solid improvements and clean up!

Copy link
Contributor

@abailly-akamai abailly-akamai left a comment

Choose a reason for hiding this comment

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

Thanks for the all the improvements here! it's going a long way in improving the tagging experience, and thank you for keeping an open mind regarding the earlier refactors.

No regression found with the latest changes ✅

⚠️ Can you make sure we get a proper e2e run before merging? I think the CI chocked on the last run

@hkhalil-akamai hkhalil-akamai merged commit 0016832 into linode:develop Apr 3, 2024
18 checks passed
@hkhalil-akamai hkhalil-akamai deleted the M3-7540-weird-tag-behavior branch April 3, 2024 19:13
bnussman-akamai pushed a commit to bnussman-akamai/manager that referenced this pull request Apr 4, 2024
* Trying some stuff...

* Introduce new MutationObserver hook

* Use new MutationObserver hook to listen for parent becoming visible

* Clean up

* Improve TagCell styles for small displays

* Added changeset: Tag overflow detection

* Improve tags experience and replace deprecated Select with Autocomplete component

* Update changeset

* Remove workaround and update changeset

* Fix test failure

* Fix race condition with new useAtomic hook

* Delete TagsPanel in favor of TagCell

* Add test for useAtomic

* Fix inconsistent TagCell margin bottom

* Feedback @abailly-akamai

* Add debouncing to useAtomic

* Remove useAtomic

* Fix errors due to forwarded prop

* Add loading state to AddTag

* Add storybook entry for TagCell

* Feedback @abailly-akamai
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Fixes for regressions or bugs Ready for Review UX/UI Changes for UI/UX to review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants