Skip to content

Conversation

@zeroedin
Copy link
Collaborator

@zeroedin zeroedin commented May 7, 2025

What I did

  1. Corrects default padding values for item menus
  2. Provides support for the Account dropdown from design, we need the ability to remove the padding from the rh-navigation-primary-item-menu container.
  3. Added --rh-navigation-primary-item-menu-padding-block-end
  4. Improved missing docs in sub-component rh-navigation-item

Testing Instructions

  1. View deploy preview demo
  2. Add a new CSS rule rh-navigation-primary-item { --rh-navigation-primary-item-menu-padding-block-end: 0; }
  3. The item dropdown menu should have zero bottom padding.

Notes to Reviewers

I think this is likely a minor change due to the addition of the new functionality, but because we are releasing nav in a controlled way right now and know our consumers, maybe we can squeeze this as a patch-level concern. Thoughts? Will add changeset when we resolve this question.

I also made the choice to document the new CSS prop on rh-navigation-item as rh-navigation-item-menu is used in the shadowroot of rh-navigation-item and would not be available to use as a selector to modify the value.

Here is an example of intended use:

rh-navigation-item#some-selector {
  --rh-navigation-primary-item-menu-padding-block-end: 0;
}

@zeroedin zeroedin added this to the 2025/Q2 — Diglett release milestone May 7, 2025
@zeroedin zeroedin requested a review from bennypowers May 7, 2025 19:22
@zeroedin zeroedin self-assigned this May 7, 2025
@zeroedin zeroedin added the bug Something isn't working label May 7, 2025
@changeset-bot
Copy link

changeset-bot bot commented May 7, 2025

⚠️ No Changeset found

Latest commit: a94b03f

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@netlify
Copy link

netlify bot commented May 7, 2025

Deploy Preview for red-hat-design-system ready!

Name Link
🔨 Latest commit a94b03f
🔍 Latest deploy log https://app.netlify.com/projects/red-hat-design-system/deploys/682e140e62085d0008354ccc
😎 Deploy Preview https://deploy-preview-2360--red-hat-design-system.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify project configuration.

@github-actions
Copy link
Contributor

github-actions bot commented May 7, 2025

Size Change: +203 B (+0.09%)

Total Size: 234 kB

Filename Size Change
./elements.js 545 B +6 B (+1.11%)
./elements/rh-navigation-primary/rh-navigation-primary-item-menu.js 1.06 kB +37 B (+3.61%)
./elements/rh-navigation-primary/rh-navigation-primary-item.js 3.56 kB +160 B (+4.7%) 🔍
ℹ️ View Unchanged
Filename Size
./elements/rh-accordion/context.js 162 B
./elements/rh-accordion/rh-accordion-header.js 2.67 kB
./elements/rh-accordion/rh-accordion-panel.js 1.26 kB
./elements/rh-accordion/rh-accordion.js 3.32 kB
./elements/rh-alert/rh-alert.js 4.98 kB
./elements/rh-announcement/rh-announcement.js 2.12 kB
./elements/rh-audio-player/rh-audio-player-about.js 1.81 kB
./elements/rh-audio-player/rh-audio-player-rate-stepper.js 1.76 kB
./elements/rh-audio-player/rh-audio-player-scrolling-text-overflow.js 1.52 kB
./elements/rh-audio-player/rh-audio-player-subscribe.js 1.4 kB
./elements/rh-audio-player/rh-audio-player.js 13 kB
./elements/rh-audio-player/rh-cue.js 1.95 kB
./elements/rh-audio-player/rh-transcript.js 2.68 kB
./elements/rh-avatar/random-pattern-controller.js 2.72 kB
./elements/rh-avatar/rh-avatar.js 2.68 kB
./elements/rh-back-to-top/rh-back-to-top.js 1.96 kB
./elements/rh-badge/rh-badge.js 1.53 kB
./elements/rh-blockquote/rh-blockquote.js 1.37 kB
./elements/rh-breadcrumb/rh-breadcrumb.js 1.02 kB
./elements/rh-button/rh-button.js 3.29 kB
./elements/rh-card/rh-card.js 3.43 kB
./elements/rh-chip/context.js 165 B
./elements/rh-chip/rh-chip-group.js 1.58 kB
./elements/rh-chip/rh-chip.js 2.06 kB
./elements/rh-code-block/prism.css.js 667 B
./elements/rh-code-block/prism.js 572 B
./elements/rh-code-block/rh-code-block.js 7.24 kB
./elements/rh-cta/rh-cta.js 3.96 kB
./elements/rh-dialog/rh-dialog.js 4.81 kB
./elements/rh-dialog/yt-api.js 617 B
./elements/rh-disclosure/rh-disclosure.js 2.09 kB
./elements/rh-footer/rh-footer-block.js 714 B
./elements/rh-footer/rh-footer-copyright.js 362 B
./elements/rh-footer/rh-footer-links.js 1.19 kB
./elements/rh-footer/rh-footer-social-link.js 1.15 kB
./elements/rh-footer/rh-footer-universal.js 3.96 kB
./elements/rh-footer/rh-footer.js 4.84 kB
./elements/rh-health-index/rh-health-index.js 2.4 kB
./elements/rh-icon/rh-icon.js 2.49 kB
./elements/rh-icon/ssr.js 181 B
./elements/rh-jump-links/context.js 179 B
./elements/rh-jump-links/rh-jump-link.js 1.46 kB
./elements/rh-jump-links/rh-jump-links-list.js 1.15 kB
./elements/rh-jump-links/rh-jump-links.js 2.35 kB
./elements/rh-menu/rh-menu.js 1.21 kB
./elements/rh-navigation-primary/context.js 176 B
./elements/rh-navigation-primary/rh-navigation-primary-overlay.js 534 B
./elements/rh-navigation-primary/rh-navigation-primary.js 7.32 kB
./elements/rh-navigation-primary/test/fixtures.js 4.49 kB
./elements/rh-navigation-secondary/rh-navigation-secondary-dropdown.js 2.57 kB
./elements/rh-navigation-secondary/rh-navigation-secondary-menu-section.js 1.32 kB
./elements/rh-navigation-secondary/rh-navigation-secondary-menu.js 1.75 kB
./elements/rh-navigation-secondary/rh-navigation-secondary-overlay.js 562 B
./elements/rh-navigation-secondary/rh-navigation-secondary.js 5.26 kB
./elements/rh-navigation-secondary/test/fixtures.js 769 B
./elements/rh-pagination/rh-pagination.js 5.57 kB
./elements/rh-site-status/rh-site-status.js 2.38 kB
./elements/rh-skip-link/rh-skip-link.js 1.19 kB
./elements/rh-spinner/rh-spinner.js 1.29 kB
./elements/rh-stat/rh-stat.js 2.08 kB
./elements/rh-subnav/rh-subnav.js 2.44 kB
./elements/rh-surface/rh-surface.js 893 B
./elements/rh-surface/test/elements.js 763 B
./elements/rh-switch/rh-switch.js 2.89 kB
./elements/rh-table/rh-sort-button.js 1.4 kB
./elements/rh-table/rh-table.js 2.89 kB
./elements/rh-tabs/context.js 226 B
./elements/rh-tabs/rh-tab-panel.js 973 B
./elements/rh-tabs/rh-tab.js 2.95 kB
./elements/rh-tabs/rh-tabs.js 3.62 kB
./elements/rh-tag/rh-tag.js 2.66 kB
./elements/rh-tile/rh-tile-group.js 1.78 kB
./elements/rh-tile/rh-tile.js 4.89 kB
./elements/rh-timestamp/rh-timestamp.js 983 B
./elements/rh-tooltip/rh-tooltip.js 2.71 kB
./elements/rh-video-embed/rh-video-embed.js 4.6 kB
./lib/color-palettes.js 851 B
./lib/context/headings/consumer.js 593 B
./lib/context/headings/provider.js 1.2 kB
./lib/elements/rh-context-demo/rh-context-demo.js 1.16 kB
./lib/elements/rh-context-picker/rh-context-picker.js 2.18 kB
./lib/environment.js 194 B
./lib/functions.js 175 B
./lib/I18nController.js 1.37 kB
./lib/ScreenSizeController.js 876 B
./lib/ssr-controller.js 201 B
./lib/themable.js 549 B
./react/rh-accordion/rh-accordion-header.js 199 B
./react/rh-accordion/rh-accordion-panel.js 185 B
./react/rh-accordion/rh-accordion.js 215 B
./react/rh-alert/rh-alert.js 184 B
./react/rh-announcement/rh-announcement.js 189 B
./react/rh-audio-player/rh-audio-player-about.js 191 B
./react/rh-audio-player/rh-audio-player-rate-stepper.js 213 B
./react/rh-audio-player/rh-audio-player-scrolling-text-overflow.js 214 B
./react/rh-audio-player/rh-audio-player-subscribe.js 196 B
./react/rh-audio-player/rh-audio-player.js 183 B
./react/rh-audio-player/rh-cue.js 195 B
./react/rh-audio-player/rh-transcript.js 207 B
./react/rh-avatar/rh-avatar.js 173 B
./react/rh-back-to-top/rh-back-to-top.js 183 B
./react/rh-badge/rh-badge.js 174 B
./react/rh-blockquote/rh-blockquote.js 179 B
./react/rh-breadcrumb/rh-breadcrumb.js 179 B
./react/rh-button/rh-button.js 174 B
./react/rh-card/rh-card.js 172 B
./react/rh-chip/rh-chip-group.js 182 B
./react/rh-chip/rh-chip.js 187 B
./react/rh-code-block/rh-code-block.js 181 B
./react/rh-cta/rh-cta.js 170 B
./react/rh-dialog/rh-dialog.js 203 B
./react/rh-disclosure/rh-disclosure.js 192 B
./react/rh-footer/rh-footer-block.js 184 B
./react/rh-footer/rh-footer-copyright.js 187 B
./react/rh-footer/rh-footer-links.js 185 B
./react/rh-footer/rh-footer-social-link.js 193 B
./react/rh-footer/rh-footer-universal.js 188 B
./react/rh-footer/rh-footer.js 174 B
./react/rh-health-index/rh-health-index.js 184 B
./react/rh-icon/rh-icon.js 202 B
./react/rh-jump-links/rh-jump-link.js 196 B
./react/rh-jump-links/rh-jump-links-list.js 189 B
./react/rh-jump-links/rh-jump-links.js 195 B
./react/rh-menu/rh-menu.js 173 B
./react/rh-navigation-primary/rh-navigation-primary-item-menu.js 205 B
./react/rh-navigation-primary/rh-navigation-primary-item.js 210 B
./react/rh-navigation-primary/rh-navigation-primary-overlay.js 199 B
./react/rh-navigation-primary/rh-navigation-primary.js 189 B
./react/rh-navigation-secondary/rh-navigation-secondary-dropdown.js 217 B
./react/rh-navigation-secondary/rh-navigation-secondary-menu-section.js 205 B
./react/rh-navigation-secondary/rh-navigation-secondary-menu.js 199 B
./react/rh-navigation-secondary/rh-navigation-secondary-overlay.js 201 B
./react/rh-navigation-secondary/rh-navigation-secondary.js 213 B
./react/rh-pagination/rh-pagination.js 178 B
./react/rh-site-status/rh-site-status.js 181 B
./react/rh-skip-link/rh-skip-link.js 181 B
./react/rh-spinner/rh-spinner.js 175 B
./react/rh-stat/rh-stat.js 171 B
./react/rh-subnav/rh-subnav.js 175 B
./react/rh-surface/rh-surface.js 175 B
./react/rh-switch/rh-switch.js 185 B
./react/rh-table/rh-sort-button.js 213 B
./react/rh-table/rh-table.js 174 B
./react/rh-tabs/rh-tab-panel.js 181 B
./react/rh-tabs/rh-tab.js 187 B
./react/rh-tabs/rh-tabs.js 174 B
./react/rh-tag/rh-tag.js 182 B
./react/rh-tile/rh-tile-group.js 183 B
./react/rh-tile/rh-tile.js 194 B
./react/rh-timestamp/rh-timestamp.js 176 B
./react/rh-tooltip/rh-tooltip.js 175 B
./react/rh-video-embed/rh-video-embed.js 227 B
./uxdot/ssr-failure-recoverable.js 581 B
./uxdot/uxdot-best-practice.js 812 B
./uxdot/uxdot-color-scheme-picker.js 1.56 kB
./uxdot/uxdot-copy-button.js 1.24 kB
./uxdot/uxdot-copy-permalink.js 1.1 kB
./uxdot/uxdot-demo.js 1.38 kB
./uxdot/uxdot-example.js 1.14 kB
./uxdot/uxdot-feedback.js 727 B
./uxdot/uxdot-header.js 1.02 kB
./uxdot/uxdot-masthead.js 1.25 kB
./uxdot/uxdot-pattern-ssr-controller-client.js 615 B
./uxdot/uxdot-pattern-ssr-controller-server.js 1.71 kB
./uxdot/uxdot-pattern-ssr-controller.js 213 B
./uxdot/uxdot-pattern.js 2.23 kB
./uxdot/uxdot-repo-status-checklist.js 1.16 kB
./uxdot/uxdot-repo-status-list.js 1.07 kB
./uxdot/uxdot-repo-status-table.js 782 B
./uxdot/uxdot-repo.js 1.17 kB
./uxdot/uxdot-search.js 2.39 kB
./uxdot/uxdot-sidenav.js 2.69 kB
./uxdot/uxdot-spacer-tokens-table.js 2.46 kB
./uxdot/uxdot-toc.js 1.8 kB

compressed-size-action

@bennypowers
Copy link
Member

Some thoughts on how we might patch this without adding any new CSS:

  • How tight are the constraints on the content which needs this feature? In other words, is this a variation of the "disclosure host styles when there's a jump links inside" problem?
  • What if we invert things and remove the padding on the container for everyone, but then add margins to the content? How annoying would that be?

@zeroedin
Copy link
Collaborator Author

zeroedin commented May 9, 2025

Some thoughts on how we might patch this without adding any new CSS:

  • How tight are the constraints on the content which needs this feature? In other words, is this a variation of the "disclosure host styles when there's a jump links inside" problem?

In the future, yes/maybe, given @MagRat04 work on rhx-account-dropdown (working title), that would be the content region of the account dropdown specifically. Now, I can't speak to his work and alignment with what we see design-wise in the rh-navigation-primary figma. But my general understanding is this is or could be a case of this content is inserted into this parent component, now we need the parent to do something different then normal, maybe preferably without any additional configuration.

Right now, it's just slotted standard HTML content from drupal

  • What if we invert things and remove the padding on the container for everyone, but then add margins to the content? How annoying would that be?

I'm hesitant on this. That would mean for all dropdowns, whereas this change is really for just 1. The default is you have padding. The account dropdown right now is the exception.

@zeroedin
Copy link
Collaborator Author

zeroedin commented May 21, 2025

The padding is also ONLY removed in the "unauthed" state.

Authed:
image

Unauthed:
image

The original slot's intent was to maintain the padding across all the dropdowns, which was also because those dropdowns hadn't been designed when the component was created.

I'm wondering, given @MagRat04's work, if we can use a check to see if the rhx-account-dropdown is slotted as we suggest in rh-disclosure for rh-jump-links and then remove the bottom padding.

For RHDC purposes, because they likely won't use rhx-account-dropdown, I wonder if they would find it acceptable to use rhx-account-dropdown as a custom-element wrapper like div to trigger the same outcomes... 🤔 That said, I also wonder the barriers to adoption of rhx-account-dropdown across the board.

Update: "RHDC purposes, because they likely won't use". They do use the account dropdown, so this would be a viable option.

@bennypowers
Copy link
Member

I'd prefer if we get some clarity on the allowed content from our stakeholders before proceeding with this

@markcaron
Copy link
Collaborator

@zeroedin @bennypowers where are we at on this fix? Will it make it into Diglett?

@markcaron markcaron moved this to Hold 🔴 in Red Hat Design System Jun 26, 2025
@bennypowers
Copy link
Member

@markcaron no we can't move on this until PMs come in with a clearly delineated business case e.g. "when there's a login avatar in the menu, remove the padding" or something like that

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

Status: Hold 🔴

Development

Successfully merging this pull request may close these issues.

5 participants