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.

Sign up for GitHub

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

Jump to bottom

feat(pagination): Add CSS vars for $pagination #1268

Merged
merged 6 commits into from
Jul 22, 2022
Merged

feat(pagination): Add CSS vars for $pagination #1268

merged 6 commits into from
Jul 22, 2022

Conversation

isabellechanclou
Copy link
Member

@isabellechanclou isabellechanclou commented May 19, 2022 •
edited by julien-deramond

Add CSS vars in Pagination component.

@isabellechanclou isabellechanclou self-assigned this May 19, 2022
@isabellechanclou isabellechanclou mentioned this pull request May 19, 2022
Closed
75 tasks
@netlify
Copy link

netlify bot commented May 19, 2022 •
edited

✅ Deploy Preview for boosted ready!

Name Link
🔨 Latest commit 8a28336
🔍 Latest deploy log https://app.netlify.com/sites/boosted/deploys/62da938ab674c30008dd7819
😎 Deploy Preview https://deploy-preview-1268--boosted.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 settings.

louismaximepiton
louismaximepiton requested changes May 25, 2022
View reviewed changes
Copy link
Member

@louismaximepiton louismaximepiton left a comment

Choose a reason for hiding this comment

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

Many review for Bootstrap side + Boosted mods

scss/_pagination.scss Outdated Show resolved Hide resolved
scss/_pagination.scss Show resolved Hide resolved
scss/_pagination.scss Outdated Show resolved Hide resolved
scss/_pagination.scss Outdated Show resolved Hide resolved
scss/_pagination.scss
@@ -67,17 +85,17 @@
color: var(--#{$prefix}pagination-focus-color);
background-color: var(--#{$prefix}pagination-focus-bg);
border-color: var(--#{$prefix}pagination-hover-border-color); // Boosted mod
outline: $pagination-focus-outline;
outline: var(--#{$prefix}pagination-focus-outline); // Boosted mod
Copy link
Member

Choose a reason for hiding this comment

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

Should be changed on Bs side too ?

Copy link
Member

Choose a reason for hiding this comment

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

Haven't looked at it but for this one and https://github.com/Orange-OpenSource/Orange-Boosted-Bootstrap/pull/1268/files#r881440526, @isabellechanclou you could create a PR for Bootstrap to propose those modifications.

scss/_pagination.scss
@@ -101,7 +119,7 @@

.page-item {
&:not(:first-child) .page-link {
margin-left: $pagination-margin-start;
margin-left: var(--#{$prefix}pagination-margin-start); // Boosted mod
Copy link
Member

Choose a reason for hiding this comment

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

Bs side too ?

scss/_pagination.scss Outdated Show resolved Hide resolved
scss/_pagination.scss Outdated Show resolved Hide resolved
scss/_pagination.scss Outdated Show resolved Hide resolved
@isabellechanclou isabellechanclou force-pushed the main-ic-css-variables-pagination branch from 1f842b9 to f4348dc Compare May 30, 2022 14:25
louismaximepiton
louismaximepiton approved these changes May 30, 2022
View reviewed changes
Copy link
Member

@louismaximepiton louismaximepiton left a comment

Choose a reason for hiding this comment

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

Sorry, I missed some Boosted mods with first review

scss/_pagination.scss Outdated Show resolved Hide resolved
scss/_pagination.scss Outdated Show resolved Hide resolved
louismaximepiton
louismaximepiton approved these changes May 31, 2022
View reviewed changes
Copy link
Member

@louismaximepiton louismaximepiton left a comment

Choose a reason for hiding this comment

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

LGTM, few questions to answer before merging

@julien-deramond
Copy link
Member

LGTM, few questions to answer before merging

What's the status of this PR. Approved on your side @louismaximepiton?

@louismaximepiton
Copy link
Member

Yup the PR is fine to me, but we need to propose some changes to Bs directly, since some replaced variables still exist in Bootstrap with Sass only.

julien-deramond
julien-deramond requested changes Jun 22, 2022
View reviewed changes
Copy link
Member

@julien-deramond julien-deramond left a comment •
edited

Choose a reason for hiding this comment

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

I let you modify the order of the CSS vars and remove the line breaks. I'll review it more in depth after. It'll be easier to review it like that for me.

@isabellechanclou isabellechanclou force-pushed the main-ic-css-variables-pagination branch from eb67518 to fedcb28 Compare June 23, 2022 14:17
@julien-deramond julien-deramond self-requested a review June 27, 2022 07:17
@isabellechanclou
feat(pagination): Add CSS vars for $pagination
6409653 
Signed-off-by: Isabelle Chanclou <isabelle.chanclou@orange.com>
@isabellechanclou
Fixes after 1st review
d199d74 
Signed-off-by: Isabelle Chanclou <isabelle.chanclou@orange.com>
@isabellechanclou
Delete extra Boosted mod comments
47e21f8 
Signed-off-by: Isabelle Chanclou <isabelle.chanclou@orange.com>
@isabellechanclou
Change CSS variable order to match Boostrap's one
5cbc68c 
Signed-off-by: Isabelle Chanclou <isabelle.chanclou@orange.com>
@isabellechanclou isabellechanclou force-pushed the main-ic-css-variables-pagination branch from a56e54f to 5cbc68c Compare July 20, 2022 07:39
julien-deramond
julien-deramond approved these changes Jul 22, 2022
View reviewed changes
Copy link
Member

@julien-deramond julien-deramond left a comment

Choose a reason for hiding this comment

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

LGTM! Renamed --#{$prefix}pagination-active-item-border in --#{$prefix}pagination-active-item-border-color and reorder them.

Regarding new CSS vars, IMO they could be added in Bootstrap as well. However IDK if they should be all in .pagination, so let's try it and get some feedback.

@julien-deramond julien-deramond merged commit 1047f02 into main Jul 22, 2022
@julien-deramond julien-deramond deleted the main-ic-css-variables-pagination branch July 22, 2022 12:15
@julien-deramond julien-deramond mentioned this pull request Jul 22, 2022
Closed
@isabellechanclou isabellechanclou mentioned this pull request Aug 11, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@julien-deramond julien-deramond julien-deramond approved these changes

@louismaximepiton louismaximepiton louismaximepiton approved these changes

Assignees

@isabellechanclou isabellechanclou

Labels
css enhancement v5
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@isabellechanclou @julien-deramond @louismaximepiton