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

feat: pagination link component (resolves #10) #29

Merged
merged 12 commits into from
Jul 25, 2023

Conversation

chosww
Copy link
Collaborator

@chosww chosww commented Jul 21, 2023

See #10 for details.

@chosww chosww added the enhancement New feature or request label Jul 21, 2023
@chosww chosww added this to the 1.0.0 milestone Jul 21, 2023
@chosww chosww self-assigned this Jul 21, 2023
@netlify
Copy link

netlify bot commented Jul 21, 2023

Deploy Preview for idg-ds ready!

Name Link
🔨 Latest commit 9bf74b7
🔍 Latest deploy log https://app.netlify.com/sites/idg-ds/deploys/64bfdbcc4d69310008515bd8
😎 Deploy Preview https://deploy-preview-29--idg-ds.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.

@chosww chosww requested a review from greatislander July 21, 2023 19:23
@chosww chosww linked an issue Jul 21, 2023 that may be closed by this pull request
Copy link
Member

@greatislander greatislander left a comment

Choose a reason for hiding this comment

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

A couple changes to the gradient. Also, this still needs focus and active states!

src/static/css/_pagination.css Outdated Show resolved Hide resolved
src/static/css/_pagination.css Outdated Show resolved Hide resolved
src/static/css/_pagination.css Outdated Show resolved Hide resolved
src/static/css/_pagination.css Outdated Show resolved Hide resolved
Copy link
Member

@greatislander greatislander left a comment

Choose a reason for hiding this comment

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

A couple changes to the gradient. Also, this still needs focus and active states!

@chosww
Copy link
Collaborator Author

chosww commented Jul 21, 2023

A couple changes to the gradient. Also, this still needs focus and active states!

Thanks for your review! I got the gradient values from Figma, so I assumed that would be correct. I forgot about the focus and active styles, I will add them now.

@chosww chosww requested a review from greatislander July 21, 2023 20:14
@greatislander
Copy link
Member

A couple changes to the gradient. Also, this still needs focus and active states!

Thanks for your review! I got the gradient values from Figma, so I assumed that would be correct. I forgot about the focus and active styles, I will add them now.

We were both right with respect to the colours— the gradients actually go to color-blue-100, not white. I converted the RGB to HSL and it matched. Can you make that change?

@greatislander
Copy link
Member

One issue with the focus state is that the focus ring as shown in the design should be inside the box and not outside.

You can see an example of how to address this using box-shadow on my draft PR for navigation:

https://github.com/inclusive-design/idg-design-system/blob/feat/navigation/src/static/css/_navigation.css#L19

box-shadow: 0 0 0 0.375rem var(--fl-linkColor, var(--nav-item-border)) inset;

Since you won't be using the outline for focus styles, you can set its colour to transparent.

@greatislander greatislander enabled auto-merge (squash) July 24, 2023 11:45
Copy link
Member

@greatislander greatislander left a comment

Choose a reason for hiding this comment

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

One other note— even in the default state I think the top border and color should use --fl-linkColor to show that it's an interactive element.

@chosww
Copy link
Collaborator Author

chosww commented Jul 24, 2023

One issue with the focus state is that the focus ring as shown in the design should be inside the box and not outside.

You can see an example of how to address this using box-shadow on my draft PR for navigation:

https://github.com/inclusive-design/idg-design-system/blob/feat/navigation/src/static/css/_navigation.css#L19

box-shadow: 0 0 0 0.375rem var(--fl-linkColor, var(--nav-item-border)) inset;

Since you won't be using the outline for focus styles, you can set its colour to transparent.

Thanks for your feedback! I set outline to "none" instead, since we don't use it, but if you think setting it to transparent is more appropriate, I will change it to transparent. Also, I added align-items center to the pagination class, because I noticed that the text in the button is not vertically centered.

@greatislander
Copy link
Member

Thanks for your feedback! I set outline to "none" instead, since we don't use it, but if you think setting it to transparent is more appropriate, I will change it to transparent. Also, I added align-items center to the pagination class, because I noticed that the text in the button is not vertically centered.

The reason for the outline rule is described in the link below— and actually you should follow the syntax shown here (outline: 3px solid transparent;): https://sarahmhigley.com/writing/whcm-quick-tips/#1.-custom-focus-styles-%2B-a-transparent-outline

@chosww
Copy link
Collaborator Author

chosww commented Jul 24, 2023

Thanks for your feedback! I set outline to "none" instead, since we don't use it, but if you think setting it to transparent is more appropriate, I will change it to transparent. Also, I added align-items center to the pagination class, because I noticed that the text in the button is not vertically centered.

The reason for the outline rule is described in the link below— and actually you should follow the syntax shown here (outline: 3px solid transparent;): https://sarahmhigley.com/writing/whcm-quick-tips/#1.-custom-focus-styles-%2B-a-transparent-outline

Thanks for your explanation and the link!

@greatislander
Copy link
Member

greatislander commented Jul 24, 2023

@chosww You need to use the same approach for both the default style and the focus style— box-shadow. Right now this happens (notice the jump of the top border):

focus.mp4

You can see this on my navigation PR. You may also want to explore the box-shadow syntax on MDN: https://developer.mozilla.org/en-US/docs/Web/CSS/box-shadow

src/static/css/_pagination.css Outdated Show resolved Hide resolved
src/static/css/_pagination.css Outdated Show resolved Hide resolved
src/static/css/_pagination.css Outdated Show resolved Hide resolved
src/_includes/components/pagination/pagination.njk Outdated Show resolved Hide resolved
src/static/css/_pagination.css Outdated Show resolved Hide resolved
src/static/css/_pagination.css Outdated Show resolved Hide resolved
src/static/css/_pagination.css Outdated Show resolved Hide resolved
src/static/css/_variables.css Outdated Show resolved Hide resolved
@chosww chosww requested a review from greatislander July 25, 2023 13:42
Copy link
Member

@greatislander greatislander left a comment

Choose a reason for hiding this comment

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

Final change: the pagination link should not be underlined, so you'll need to add a CSS rule to remove the underline.

@greatislander greatislander merged commit 314f1f8 into inclusive-design:main Jul 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Component: pagination link
2 participants