Skip to content

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

Closed
peterkulko wants to merge 6 commits into
openedx:masterfrom
raccoongang:Peter_Kulko/pagination-component-fix
Closed

[BD-46] DRAFT: fixed pagination buttons#1849
peterkulko wants to merge 6 commits into
openedx:masterfrom
raccoongang:Peter_Kulko/pagination-component-fix

Conversation

@peterkulko

@peterkulko peterkulko commented Dec 15, 2022

Copy link
Copy Markdown
Contributor

Description

  • Ensure the Pagination buttons no longer use .btn-primary.
  • Ensure the non-active pages of Pagination have a transparent background (they are currently white). This will help make Pagination usable on a wider array of background colors than just white.

image

Deploy Preview

Pagination component

Merge Checklist

  • If your update includes visual changes, have they been reviewed by a designer? Send them a link to the Netlify deploy preview, if applicable.
  • Does your change adhere to the documented style conventions?
  • Do any prop types have missing descriptions in the Props API tables in the documentation site (check deploy preview)?
  • Were your changes tested using all available themes (see theme switcher in the header of the deploy preview, under the "Settings" icon)?
  • Were your changes tested in the example app?
  • Is there adequate test coverage for your changes?
  • Consider whether this change needs to reviewed/QA'ed for accessibility (a11y). If so, please add wittjeff and adamstankiewicz as reviewers on this PR.

Post-merge Checklist

  • Verify your changes were released to NPM at the expected version.
  • If you'd like, share your contribution in #show-and-tell.
  • 🎉 🙌 Celebrate! Thanks for your contribution.

@openedx-webhooks openedx-webhooks added the blended PR is managed through 2U's blended developmnt program label Dec 15, 2022
@openedx-webhooks

Copy link
Copy Markdown

Thanks for the pull request, @PKulkoRaccoonGang!

When this pull request is ready, tag your edX technical lead.

@netlify

netlify Bot commented Dec 15, 2022

Copy link
Copy Markdown

Deploy Preview for paragon-openedx ready!

Built without sensitive environment variables

Name Link
🔨 Latest commit 12ae193
🔍 Latest deploy log https://app.netlify.com/sites/paragon-openedx/deploys/63a9531803baf20007bacbda
😎 Deploy Preview https://deploy-preview-1849--paragon-openedx.netlify.app/components/pagination
📱 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.

@peterkulko peterkulko changed the title [BD-46] fix: fixed pagination buttons [BD-46] fix: fixed pagination buttons in Pagination components Dec 15, 2022
@peterkulko peterkulko changed the title [BD-46] fix: fixed pagination buttons in Pagination components [BD-46] fix: fixed pagination buttons Dec 15, 2022
@codecov

codecov Bot commented Dec 15, 2022

Copy link
Copy Markdown

Codecov Report

Base: 90.79% // Head: 91.03% // Increases project coverage by +0.24% 🎉

Coverage data is based on head (12ae193) compared to base (b84a331).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1849      +/-   ##
==========================================
+ Coverage   90.79%   91.03%   +0.24%     
==========================================
  Files         212      214       +2     
  Lines        3757     3794      +37     
  Branches      892      908      +16     
==========================================
+ Hits         3411     3454      +43     
+ Misses        344      338       -6     
  Partials        2        2              
Impacted Files Coverage Δ
src/Pagination/index.jsx 96.45% <100.00%> (+0.15%) ⬆️
src/Card/index.jsx 100.00% <0.00%> (ø)
src/Bubble/index.tsx 100.00% <0.00%> (ø)
src/Truncate/utils.js 100.00% <0.00%> (ø)
src/Truncate/index.jsx 100.00% <0.00%> (ø)
src/Breadcrumb/index.jsx 100.00% <0.00%> (ø)
src/Card/CardContext.jsx 100.00% <0.00%> (ø)
src/Card/CardDeck.jsx 100.00% <0.00%> (ø)
src/Breadcrumb/BreadcrumbLink.jsx 100.00% <0.00%> (ø)
... and 2 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

Comment thread src/Pagination/index.jsx Outdated
<Button
className="page-link"
aria-label={ariaLabel}
variant={active ? 'primary' : 'tertiary'}

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

Comment thread src/Pagination/Pagination.scss Outdated

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

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).

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

@peterkulko
peterkulko force-pushed the Peter_Kulko/pagination-component-fix branch from d725044 to 259cbf0 Compare December 18, 2022 11:27
@peterkulko peterkulko changed the title [BD-46] fix: fixed pagination buttons [BD-46] DRAFT: fixed pagination buttons Dec 23, 2022
@adamstankiewicz

Copy link
Copy Markdown
Member

Closing this PR as I believe it is superseded by #1911

@openedx-webhooks

Copy link
Copy Markdown

@PKulkoRaccoonGang Even though your pull request wasn’t merged, please take a moment to answer a two question survey so we can improve your experience in the future.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

blended PR is managed through 2U's blended developmnt program

Projects

No open projects
Status: Done

Development

Successfully merging this pull request may close these issues.

Pagination component includes .btn-primary on all its buttons, when only the active page actually has .btn-primary styling

4 participants