Skip to content

[BD-46] DRAFT: fixed pagination buttons #1849

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

Closed
peterkulko wants to merge 6 commits into from
Closed

[BD-46] DRAFT: fixed pagination buttons #1849

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
cdab705
fix: fixed pagination buttons
peterkulko Dec 15, 2022
32264c1
refactor: refactoring SCSS styles
peterkulko Dec 16, 2022
259cbf0
refactor: refactoring styles after review
peterkulko Dec 18, 2022
66b95c5
refactor: added some styles
peterkulko Dec 22, 2022
706b601
refactor: refactoring Pagination styles
peterkulko Dec 23, 2022
12ae193
refactor: disabled unused variables
peterkulko Dec 26, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
49 changes: 17 additions & 32 deletions src/Pagination/Pagination.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,14 +1,6 @@
@import "variables";
@import "~bootstrap/scss/pagination";

.pagination {
align-items: center;

.dropdown {
z-index: 4;
}
}

%pagination-icon-button-right {
border-top-right-radius: 50%;
border-bottom-right-radius: 50%;
Expand All @@ -19,12 +11,12 @@
border-bottom-left-radius: 50%;
}

.pagination-icon-button-right {
@extend %pagination-icon-button-right;
}
.pagination {
align-items: center;

.pagination-icon-button-left {
@extend %pagination-icon-button-left;
.dropdown {
z-index: 4;
}
}

.pagination-default {
Expand All @@ -39,26 +31,19 @@
margin-inline-end: 0;
}
}

.page-item {
&:first-child .page-link {
[dir="rtl"] & {
border-radius: 0 $pagination-border-radius-lg $pagination-border-radius-lg 0;
}
}

&:last-child .page-link {
[dir="rtl"] & {
border-radius: $pagination-border-radius-lg 0 0 $pagination-border-radius-lg;
}
}
}
}

.page-link {
border: none;
background-color: transparent;

@adamstankiewicz adamstankiewicz Dec 16, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

[curious] Is this property overriding an existing background-color property that's coming from somewhere? I see in the inspector that .page-link has background-color: #fff;, which is overridden by this line.

Should we be relying more on the stock button variant styles in Pagination instead? E.g., now that we're pulling in the "tertiary" button variant, that already has a transparent background on it.

I believe introducing this property here may be overriding the default hover/focus styles for the buttons in Pagination as well (e.g., there are no hover styles on the tertiary buttons in the inverse pallete).

@viktorrusakov viktorrusakov Dec 21, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

yeah, I think you're right, we'll take a closer look at this

color: $pagination-page-link-color;

&.btn-primary:not(:disabled):not(.disabled):focus {
&:hover {
color: $pagination-page-link-color;
}

&.btn-primary:not(:disabled):not(.disabled):focus,
&.btn-tertiary:not(:disabled):not(.disabled):focus {
background-color: $pagination-bg;
color: $pagination-focus-color-text;
}
Expand All @@ -67,10 +52,10 @@
box-shadow: none;
}

&.btn-primary:focus::before {
&.btn-primary:focus::before,
&.btn-tertiary:focus::before {
border: $pagination-focus-border-width solid $pagination-focus-color;

@include button-size($btn-padding-y, $btn-padding-x, $btn-font-size, $btn-line-height, $btn-border-radius);
border-radius: $btn-border-radius;
}

div {
Expand Down Expand Up @@ -171,7 +156,7 @@

&.active button.page-link {
background-color: $pagination-bg;
color: $pagination-color;
color: $pagination-page-link-color;
}

button.page-link {
Expand Down
1 change: 1 addition & 0 deletions src/Pagination/_variables.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ $pagination-secondary-height-sm: 2.25rem !default;

$pagination-color: $link-color !default;
$pagination-color-inverse: $white !default;
$pagination-page-link-color: $gray-700 !default;
$pagination-bg: $white !default;
$pagination-border-width: $border-width !default;
$pagination-border-color: theme-color("gray", "border") !default;
Expand Down
1 change: 1 addition & 0 deletions src/Pagination/index.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -158,6 +158,7 @@ class Pagination extends React.Component {
<Button
className="page-link"
aria-label={ariaLabel}
variant={active ? 'primary' : 'tertiary'}

@adamstankiewicz adamstankiewicz Dec 16, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think this is the right approach here, though I think there are some custom existing Pagination styles interfering with the expected button styles still. For example:

image

The text color of the non-active "tertiary" button variants seem to be getting a blue color instead of the standard "tertiary" button variant styles:

image

ref={(element) => { this.pageRefs[page] = element; }}
onClick={() => { this.handlePageSelect(page); }}
>
Expand Down