-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Import Pagination Component #487
Conversation
cursor: pointer; | ||
user-select: none; | ||
background: $bg-white; // Reset default gradient backgrounds and colors | ||
border: 1px solid $gray-200; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could use some more appropriate variables here:
border: $border-width $border-style $border-gray;
z-index: 2; | ||
text-decoration: none; | ||
background-color: darken($gray-100, 2%); | ||
border-color: $gray-200; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could also use $border-gray
here.
z-index: 3; | ||
color: $text-white; | ||
background-color: $bg-blue; | ||
border-color: $blue; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And $border-blue
here?
.disabled:hover { | ||
color: $gray-300; | ||
cursor: default; | ||
background-color: $gray-000; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could use $bg-gray-light
here.
<a class="next_page" rel="next" href="#url" aria-label="Next Page">Next</a> | ||
</div> | ||
</nav> | ||
``` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You need a <!-- %enddocs -->
at the end of the docs section here. That's what we look for in the styleguide to pull out the relevant doc section.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done 👍
@@ -0,0 +1,33 @@ | |||
{ | |||
"version": "0.0.1", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will probably want to start the versioning at 1.0.0
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok!
@@ -22,6 +22,7 @@ | |||
@import "primer-forms/index.scss"; | |||
@import "primer-layout/index.scss"; | |||
@import "primer-navigation/index.scss"; | |||
@import "primer-pagination/index.scss"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How does everyone feel about pagination in core vs product? I could go either way, but wanted to bring up the question. If it's not important for marketing then we can just keep it in product.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Curious about this one too - wasn't sure where to put Box Overlay either 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I assume that Marketing users could use both Pagination and Box Overlay, but happy to defer to @sophshep.
This reverts commit 64925bb.
Fixes: #421
Adds pagination component into primer 🎉 I messed up the commit history in my previous PR so started a fresh branch and cherrypicked the correct commits :)
See #485 (comment) regarding updating some of the hard coded margin & padding styles... It might be best to save that work for a separate PR to avoid major visual changes to the component
Also see #485 (comment) in the previous PR... IMO it would still be worth it to create a new component instead of reusing the btn classes since the development process would be simplified a bit by using the pagination classes (no need to add extra classes to the page links, etc)
/cc @primer/ds-core