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

Pager styled with design tokens #106

Merged
merged 5 commits into from
Sep 21, 2023
Merged

Conversation

mariannuar
Copy link
Collaborator

Description

This PR styled the Pager component with design tokens

How to review this pull request

  • Run npm run develop
  • Visit the component
  • Confirm it looks as expected in the design

Closes #96

@mariannuar mariannuar changed the title feat(pager): pager styled with design tokens Pager styled with design tokens Sep 12, 2023
@mariannuar mariannuar self-assigned this Sep 12, 2023
@mariannuar mariannuar added the 👍 Ready for Review Work is ready for review. label Sep 12, 2023
Copy link
Contributor

@amazingrando amazingrando left a comment

Choose a reason for hiding this comment

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

@mariannuar I have a couple of questions that I am curious about. This code is approved. :shipit:

* @see template_preprocess_pager()
*/
#}
{% set pager__base_class = 'pager' %}
Copy link
Contributor

Choose a reason for hiding this comment

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

What do you think about making this pager__base_class = pager__base_class|default('pager') to make it easy to override the base class?

{% if items.previous %}
<li {{ bem('item', ['prev'], pager__base_class) }}>
<a {{ bem('link', ['prev'], pager__base_class) }} href="{{ items.previous.href }}" title="{{ 'Go to previous page'|t }}" rel="prev"{{ items.previous.attributes|without('href', 'title', 'rel') }}>
<span {{ bem('visually-hidden') }}>{{ 'Previous page'|t }}</span>
Copy link
Contributor

Choose a reason for hiding this comment

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

If we are only adding a single class, should the span be <span class="visually-hidden"> without the bem function? No need to change it. I'm just curious about the answer to the question.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@amazingrando Oh! I just copy-paste what it was already of the compound components. That's why it's like that, but you're right

@amazingrando amazingrando added 🎉 Passes Code Review Code is approved by the reviewer. 👁 Review in progress Under review. 🎉 Passes Functional Review Functionality is approved by the reviewer. 🎉 Ready to Merge Functionality is approved by the reviewer. and removed 👍 Ready for Review Work is ready for review. 👁 Review in progress Under review. labels Sep 15, 2023
@amazingrando amazingrando merged commit 339fa87 into main Sep 21, 2023
@amazingrando amazingrando deleted the pager-styled-with-design-tokens branch September 21, 2023 13:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🎉 Passes Code Review Code is approved by the reviewer. 🎉 Passes Functional Review Functionality is approved by the reviewer. 🎉 Ready to Merge Functionality is approved by the reviewer.
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Pager styled with design tokens
2 participants