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

[BD-46] fix: fixed border-radius of buttons with sizes lg and sm #2786

Merged

Conversation

PKulkoRaccoonGang
Copy link
Contributor

@PKulkoRaccoonGang PKulkoRaccoonGang commented Nov 7, 2023

Description

  • updated the border-radius on the focus outline radius;
  • verified the 2px offset for the focus outline;
  • refactoring was carried out in the file README.md.

Issue: #2550

Deploy Preview

Button 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 Nov 7, 2023
@openedx-webhooks
Copy link

Thanks for the pull request, @PKulkoRaccoonGang!

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

Copy link

netlify bot commented Nov 7, 2023

Deploy Preview for paragon-openedx ready!

Built without sensitive environment variables

Name Link
🔨 Latest commit 647393e
🔍 Latest deploy log https://app.netlify.com/sites/paragon-openedx/deploys/658318bd4d992a000883f55d
😎 Deploy Preview https://deploy-preview-2786--paragon-openedx.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.

Copy link

codecov bot commented Nov 7, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (40aea5e) 92.83% compared to head (647393e) 92.87%.
Report is 8 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2786      +/-   ##
==========================================
+ Coverage   92.83%   92.87%   +0.03%     
==========================================
  Files         235      235              
  Lines        4245     4268      +23     
  Branches     1032     1033       +1     
==========================================
+ Hits         3941     3964      +23     
  Misses        300      300              
  Partials        4        4              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

src/Button/README.md Outdated Show resolved Hide resolved
@PKulkoRaccoonGang
Copy link
Contributor Author

@adamstankiewicz In this PR, the change Outline-primary border to $primary-500 was not implemented.
At the moment we have associated logic for all such buttons.
image

We also have a SCSS map, where we use the “focus” key in the open-edx theme.
image

If we need to change the stroke color, we need to change it for all the buttons. In this case, we will get the monotony that we have now (using "focus": "300").

Do you think we should change the outline color of all buttons from $primary-300 to $primary-500?

@viktorrusakov viktorrusakov linked an issue Nov 9, 2023 that may be closed by this pull request
@PKulkoRaccoonGang PKulkoRaccoonGang force-pushed the Peter_Kulko/btn-border-radius branch 2 times, most recently from 9fee659 to 4e3d8ba Compare November 10, 2023 13:18
Comment on lines -25 to -29
<Button variant="brand" className="mb-2 mb-sm-0">Brand</Button>
<Button variant="outline-brand" className="mb-2 mb-sm-0">Outline Brand</Button>
<Button variant="primary" className="mb-2 mb-sm-0">Primary</Button>
<Button variant="outline-primary" className="mb-2 mb-sm-0">Outline Primary</Button>
<Button variant="tertiary" className="mb-2 mb-sm-0">Tertiary</Button>
Copy link
Member

Choose a reason for hiding this comment

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

Note: its great to remove utility classes like this as sometimes consumers tend to leave the className props in when they copy/paste components from the docs site... lol.

src/Button/README.md Outdated Show resolved Hide resolved
@adamstankiewicz
Copy link
Member

Next steps: we'll verify at the next Paragon Working Group.

@PKulkoRaccoonGang PKulkoRaccoonGang force-pushed the Peter_Kulko/btn-border-radius branch from 4e3d8ba to a921ad8 Compare December 18, 2023 08:54
@PKulkoRaccoonGang
Copy link
Contributor Author

PKulkoRaccoonGang commented Dec 20, 2023

@adamstankiewicz At the Paragon working group, we agreed on changing the stroke color for buttons when the focus is from 300 to 500.

@viktorrusakov viktorrusakov merged commit b75b866 into openedx:master Dec 21, 2023
10 checks passed
@openedx-semantic-release-bot

🎉 This PR is included in version 21.11.4 🎉

The release is available on:

Your semantic-release bot 📦🚀

@openedx-semantic-release-bot

🎉 This PR is included in version 22.0.0-alpha.24 🎉

The release is available on:

Your semantic-release bot 📦🚀

khudym pushed a commit to khudym/paragon that referenced this pull request Jan 2, 2024
* fix: fixed border-radius of buttons with sizes lg and sm

* refactor: refactoring README file

* refactor: refactoring after review

* refactor: changed button focus stroke color
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 released on @alpha released
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

[BD-46] Border-radius of the Button component with the small size
7 participants