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

fix(menu)!: reimplement text truncating #2299

Merged
merged 14 commits into from
Jan 9, 2024

Conversation

jenndiaz
Copy link
Contributor

@jenndiaz jenndiaz commented Nov 13, 2023

Description

BREAKING CHANGE

"Previously the spectrum-Menu-itemLabel--wrapping class could be added to the spectrum-Menu-itemLabel element in a Menu Item to prevent it from wrapping and force it to use an ellipsis instead."

Fix:

  • the css to truncate was not able to be applied because spectrum-Menu-itemLabel had display: flex;
    read more about flex causing issues with truncate
  • I was able to remove this by changing the mark up of the Multi Selection with icons example which had the SVG with in the label
  • Rename classes to indicate truncating and not wrapping
  • removed any html that was making it appear that the text with truncating ...

Docs:

  • added example to docs site
  • added example to Storybook
  • added control to Storybook to toggle on all variants

How and where has this been tested?

Please tag yourself on the tests you've marked complete to confirm the tests have been run by someone other than the author.

Validation steps

  • truncating example visible on docs page
  • storybook has truncate story with each menu item over 150px truncates and displays ...
  • each story for menu has a truncate control which adds a max width to the menu and adds spectrum-Menu-itemLabel--truncate to each menu item or heading

Regression testing

Validate:

  1. The documentation pages for at least two other components are still loading, including:
  • The pages render correctly, are accessible, and are responsive.
  1. If components have been modified, VRTs have been run on this branch:
  • VRTs have been run and looked at.
  • Any VRT changes have been accepted (by reviewer and/or PR author), or there are no changes.

Screenshots

To-do list

  • I have read the contribution guidelines.
  • I have updated relevant storybook stories and templates.
  • I have tested these changes in Windows High Contrast mode.
  • If my change impacts other components, I have tested to make sure they don't break.
  • If my change impacts documentation, I have updated the documentation accordingly.
  • ✨ This pull request is ready to merge. ✨

@jenndiaz jenndiaz force-pushed the jenndiaz/css-626-menu-text-wrapping branch from df24e4e to 1daa17f Compare November 14, 2023 16:09
Copy link
Contributor

github-actions bot commented Nov 14, 2023

🚀 Deployed on https://pr-2299--spectrum-css.netlify.app

@jenndiaz jenndiaz added the run_vrt For use on PRs looking to kick off VRT label Nov 14, 2023
Copy link
Contributor

github-actions bot commented Nov 14, 2023

File metrics

Summary

Total size: 3.97 MB*
Total change (Δ): ⬆ 0.55 KB (0.01%)
Table reports on changes to a package's main file. Other changes can be found in the collapsed "Details" below.

Package Size Δ
menu 39.18 KB ⬆ 0.18 KB
Details

menu

File Head Base Δ
index-base.css 39.18 KB 39.00 KB ⬆ 0.18 KB (0.46%)
index-theme.css < 0.01 KB < 0.01 KB -
index-vars.css 39.18 KB 39.00 KB ⬆ 0.18 KB (0.46%)
index.css 39.18 KB 39.00 KB ⬆ 0.18 KB (0.46%)
mods.json 3.33 KB 3.33 KB -
themes/express.css < 0.01 KB < 0.01 KB -
themes/spectrum.css < 0.01 KB < 0.01 KB -
* Size determined by adding together the size of the main file for all packages in the library.
* Results are not gzipped or minified.
* An ASCII character in UTF-8 is 8 bits or 1 byte.

@jenndiaz jenndiaz removed the run_vrt For use on PRs looking to kick off VRT label Nov 15, 2023
@@ -369,11 +369,11 @@ governing permissions and limitations under the License.
display: grid;
/* stylelint-disable declaration-block-no-redundant-longhand-properties */
grid-template-areas:
". chevronAreaCollapsible . iconArea sectionHeadingArea . . . "
". chevronAreaCollapsible . headingIconArea sectionHeadingArea . . . "
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this previously had two iconAreas which causes some issues with menu item icons and labels not being on the correct row with in the grid.

@@ -528,8 +540,6 @@ governing permissions and limitations under the License.

.spectrum-Menu-itemLabel {
grid-area: labelArea;
display: flex;
align-items: flex-start;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Flex was causing text-overflow: ellipsis; to not work, was able to refactor the itemLabel to not need to rely on flex. This is what appeared to of broken the truncating functionality.

@jenndiaz jenndiaz marked this pull request as ready for review November 17, 2023 18:04
Copy link
Collaborator

@mdt2 mdt2 left a comment

Choose a reason for hiding this comment

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

This is looking good! A couple notes:

  • On line 1333 in components/menu/metadata/menu.yml we have one more html ... that can be removed.
  • The Storybook control doesn't do anything except for on the Truncate page, I'm wondering if we should either limit the control to only show on that Truncate page or maybe set one of the menu items on the other stories to be able to show truncating? Let me know what you think!

@jenndiaz
Copy link
Contributor Author

  • The Storybook control doesn't do anything except for on the Truncate page, I'm wondering if we should either limit the control to only show on that Truncate page or maybe set one of the menu items on the other stories to be able to show truncating? Let me know what you think!

@mdt2 Could you say more about this? It is defiantly working for me, although some are more subtle, If you adjust the Max Inline Size* it makes it more obvious. I have it setting to 150px, I could adjust that if we think it's needed.

I do think it's helpful to have the toggle on all of them, about half of them were not working previously and I think having the ability to see it in all variants could prevent it from regressing again.

@mdt2
Copy link
Collaborator

mdt2 commented Nov 17, 2023

Oh my mistake! I had only tested a few that weren't long enough for truncation. Thanks for pointing me toward the Max Inline Size, it helped me realize my error quickly.

Copy link
Collaborator

@mdt2 mdt2 left a comment

Choose a reason for hiding this comment

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

Great work Jenn!

components/menu/stories/template.js Outdated Show resolved Hide resolved
components/menu/stories/menu.stories.js Outdated Show resolved Hide resolved
components/menu/metadata/menu.yml Show resolved Hide resolved
components/menu/metadata/menu.yml Show resolved Hide resolved
@jenndiaz jenndiaz changed the title fix(menu): reimplement text truncating fix(menu)!: reimplement text truncating Nov 29, 2023
@jenndiaz jenndiaz force-pushed the jenndiaz/css-626-menu-text-wrapping branch from b160174 to 3027a29 Compare January 4, 2024 17:53
@pfulton pfulton force-pushed the jenndiaz/css-626-menu-text-wrapping branch from 8fbf51f to 6fdaf71 Compare January 9, 2024 18:47
@pfulton pfulton merged commit 9752d02 into main Jan 9, 2024
10 checks passed
@pfulton pfulton deleted the jenndiaz/css-626-menu-text-wrapping branch January 9, 2024 19:15
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.

[Menu]: No longer possible to force a Menu Item not to wrap and instead use ellipsis
4 participants