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(pagination): Colors, right to left demo, and logical properties #2116

Merged

Conversation

adamjohnson
Copy link
Collaborator

@adamjohnson adamjohnson commented Jan 10, 2025

What I did

  1. Add fallback colors to fix incorrect colors from undefined color token variables
  2. Fix background color being incorrectly applied to <a> tags in the open variant
  3. Change width/height/top/bottom etc to use logical properties
  4. Fix Right to Left demo layout

Testing Instructions

  1. First, view the rh-pagination demo on uxdot.
  2. Note the lack of proper colors
    • Active page borders
    • No border around stepper <input>
    • Link color incorrect
  3. Now view the rh-pagination demo from this DP.
    • Colors are correct as specified in the docs
  4. Compare the open variant demo on uxdot vs the open variant on this DP. Make sure it matches specs.
  5. Look at the RTL demo on uxdot (it's broken!). Now compare that to this DP's RTL demo.

Notes to Reviewers

Ensure that this is the proper way to handle adding fallback colors to things like --rh-color-interactive-primary-default.

Note: the color context demo on uxdot works because the component is wrapped in a <rh-surface>. These fallbacks are in place for when the component is not wrapped in a <rh-surface> component.

Update: we're now wrapping each pagination in a <rh-surface> component.

@adamjohnson adamjohnson added the bug Something isn't working label Jan 10, 2025
@adamjohnson adamjohnson self-assigned this Jan 10, 2025
Copy link

changeset-bot bot commented Jan 10, 2025

🦋 Changeset detected

Latest commit: 2ca2870

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@rhds/elements Patch

Not sure what this means? Click here to learn what changesets are.

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

Copy link

netlify bot commented Jan 10, 2025

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

Name Link
🔨 Latest commit 2ca2870
🔍 Latest deploy log https://app.netlify.com/sites/red-hat-design-system/deploys/6798daea9618a00008314436
😎 Deploy Preview https://deploy-preview-2116--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 site configuration.

Copy link
Contributor

github-actions bot commented Jan 10, 2025

Size Change: +3 B (0%)

Total Size: 207 kB

Filename Size Change
./elements/rh-pagination/rh-pagination.js 5.41 kB +3 B (+0.06%)
ℹ️ View Unchanged
Filename Size
./elements.js 473 B
./elements/rh-accordion/context.js 162 B
./elements/rh-accordion/rh-accordion-header.js 2.69 kB
./elements/rh-accordion/rh-accordion-panel.js 1.35 kB
./elements/rh-accordion/rh-accordion.js 3.21 kB
./elements/rh-alert/rh-alert.js 4.26 kB
./elements/rh-audio-player/rh-audio-player-about.js 1.8 kB
./elements/rh-audio-player/rh-audio-player-rate-stepper.js 1.85 kB
./elements/rh-audio-player/rh-audio-player-scrolling-text-overflow.js 1.53 kB
./elements/rh-audio-player/rh-audio-player-subscribe.js 1.38 kB
./elements/rh-audio-player/rh-audio-player.js 13.1 kB
./elements/rh-audio-player/rh-cue.js 1.95 kB
./elements/rh-audio-player/rh-transcript.js 2.7 kB
./elements/rh-avatar/random-pattern-controller.js 2.72 kB
./elements/rh-avatar/rh-avatar.js 2.85 kB
./elements/rh-back-to-top/rh-back-to-top.js 2.1 kB
./elements/rh-badge/rh-badge.js 1.55 kB
./elements/rh-blockquote/rh-blockquote.js 1.43 kB
./elements/rh-breadcrumb/rh-breadcrumb.js 1.5 kB
./elements/rh-button/rh-button.js 4.24 kB
./elements/rh-card/rh-card.js 3.58 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.34 kB
./elements/rh-cta/rh-cta.js 4.04 kB
./elements/rh-dialog/rh-dialog.js 4.78 kB
./elements/rh-dialog/yt-api.js 617 B
./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.17 kB
./elements/rh-footer/rh-footer-social-link.js 964 B
./elements/rh-footer/rh-footer-universal.js 3.99 kB
./elements/rh-footer/rh-footer.js 4.96 kB
./elements/rh-health-index/rh-health-index.js 2.35 kB
./elements/rh-icon/rh-icon.js 2.47 kB
./elements/rh-icon/ssr.js 181 B
./elements/rh-menu/rh-menu.js 1.29 kB
./elements/rh-navigation-secondary/rh-navigation-secondary-dropdown.js 2.47 kB
./elements/rh-navigation-secondary/rh-navigation-secondary-menu-section.js 1.3 kB
./elements/rh-navigation-secondary/rh-navigation-secondary-menu.js 1.75 kB
./elements/rh-navigation-secondary/rh-navigation-secondary-overlay.js 571 B
./elements/rh-navigation-secondary/rh-navigation-secondary.js 5.26 kB
./elements/rh-navigation-secondary/test/fixtures.js 769 B
./elements/rh-site-status/rh-site-status.js 2.5 kB
./elements/rh-skip-link/rh-skip-link.js 1.19 kB
./elements/rh-spinner/rh-spinner.js 1.38 kB
./elements/rh-stat/rh-stat.js 2.13 kB
./elements/rh-subnav/rh-subnav.js 2.73 kB
./elements/rh-surface/rh-surface.js 1.14 kB
./elements/rh-surface/test/elements.js 423 B
./elements/rh-switch/rh-switch.js 2.93 kB
./elements/rh-table/rh-sort-button.js 1.49 kB
./elements/rh-table/rh-table.js 3.48 kB
./elements/rh-tabs/context.js 160 B
./elements/rh-tabs/rh-tab-panel.js 1.04 kB
./elements/rh-tabs/rh-tab.js 3.02 kB
./elements/rh-tabs/rh-tabs.js 3.77 kB
./elements/rh-tag/rh-tag.js 2.79 kB
./elements/rh-tile/rh-tile-group.js 1.81 kB
./elements/rh-tile/rh-tile.js 5.09 kB
./elements/rh-timestamp/rh-timestamp.js 983 B
./elements/rh-tooltip/rh-tooltip.js 2.73 kB
./elements/rh-video-embed/rh-video-embed.js 4.59 kB
./lib/context/color/consumer.js 1.41 kB
./lib/context/color/controller.js 947 B
./lib/context/color/provider.js 2.18 kB
./lib/context/event.js 593 B
./lib/context/headings/consumer.js 722 B
./lib/context/headings/controller.js 1.12 kB
./lib/context/headings/provider.js 1.24 kB
./lib/DirController.js 565 B
./lib/elements/rh-context-demo/rh-context-demo.js 1.28 kB
./lib/elements/rh-context-picker/rh-context-picker.js 2.24 kB
./lib/environment.js 194 B
./lib/functions.js 175 B
./lib/I18nController.js 1.38 kB
./lib/ScreenSizeController.js 849 B
./lib/ssr-controller.js 251 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-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-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-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-menu/rh-menu.js 173 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/uxdot-best-practice.js 742 B
./uxdot/uxdot-copy-button.js 1.2 kB
./uxdot/uxdot-copy-permalink.js 1.1 kB
./uxdot/uxdot-example.js 1.17 kB
./uxdot/uxdot-feedback.js 727 B
./uxdot/uxdot-header.js 1.07 kB
./uxdot/uxdot-hero.js 679 B
./uxdot/uxdot-installation-tabs.js 675 B
./uxdot/uxdot-masthead.js 809 B
./uxdot/uxdot-pattern-ssr-controller-client.js 604 B
./uxdot/uxdot-pattern-ssr-controller-server.js 1.68 kB
./uxdot/uxdot-pattern-ssr-controller.js 213 B
./uxdot/uxdot-pattern.js 2.12 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.67 kB
./uxdot/uxdot-spacer-tokens-table.js 2.45 kB
./uxdot/uxdot-toc.js 1.13 kB

compressed-size-action

Copy link
Member

@bennypowers bennypowers left a comment

Choose a reason for hiding this comment

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

really like the logical properties! nice work!

Let's find a more comprehensive solution for the computed theme properties, one that doesn't require us to disable lint rules.

elements/rh-pagination/rh-pagination.css Outdated Show resolved Hide resolved
Copy link
Member

@bennypowers bennypowers left a comment

Choose a reason for hiding this comment

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

The docs pages must specify that rh-surface is required. It may be also indicated that this requirement could be relaxed in the future.

.changeset/dry-icons-love.md Outdated Show resolved Hide resolved
.changeset/dry-icons-love.md Outdated Show resolved Hide resolved
@adamjohnson
Copy link
Collaborator Author

@bennypowers See Pagination > Code in the DP for words around the requirement of <rh-surface>. PTAL, TY!

@adamjohnson adamjohnson merged commit 34b8726 into staging/cubone Jan 28, 2025
8 checks passed
@adamjohnson adamjohnson deleted the fix/pagination/colors-rtl-logical-properties branch January 28, 2025 15:13
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: Done ☑️
Development

Successfully merging this pull request may close these issues.

2 participants