[OLD] feat: Chip component redesign #2713

PKulkoRaccoonGang · 2023-10-11T16:27:50Z

Description

Chip component redesign.

Issue: #2645

Deploy Preview

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

netlify · 2023-10-11T16:27:55Z

✅ Deploy Preview for paragon-openedx ready!

Built without sensitive environment variables

Name	Link
🔨 Latest commit	`8fa7b1a`
🔍 Latest deploy log	https://app.netlify.com/sites/paragon-openedx/deploys/654ce1c9509d9000084cc0de
😎 Deploy Preview	https://deploy-preview-2713--paragon-openedx.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

openedx-webhooks · 2023-10-11T16:27:56Z

Thanks for the pull request, @PKulkoRaccoonGang! Please note that it may take us up to several weeks or months to complete a review and merge your PR.

Feel free to add as much of the following information to the ticket as you can:

supporting documentation
Open edX discussion forum threads
timeline information ("this must be merged by XX date", and why that is)
partner information ("this is a course on edx.org")
any other information that can help Product understand the context for the PR

All technical communication about the code itself will be done via the GitHub pull request interface. As a reminder, our process documentation is here.

Please let us know once your PR is ready for our review and all tests are green.

codecov · 2023-10-16T12:17:44Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (00c48ec) 92.83% compared to head (8fa7b1a) 92.87%.
Report is 4 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #2713      +/-   ##
==========================================
+ Coverage   92.83%   92.87%   +0.04%     
==========================================
  Files         235      237       +2     
  Lines        4240     4265      +25     
  Branches     1029     1036       +7     
==========================================
+ Hits         3936     3961      +25     
  Misses        300      300              
  Partials        4        4

Files	Coverage Δ
src/Chip/ChipIcon.tsx	`100.00% <100.00%> (ø)`
src/Chip/constants.js	`100.00% <100.00%> (ø)`
src/Chip/index.tsx	`100.00% <100.00%> (ø)`
src/utils/propTypes/utils.js	`94.11% <100.00%> (+3.20%)`	⬆️

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

src/Chip/mixins.scss

src/Chip/index.tsx

src/Chip/index.scss

src/Chip/index.tsx

monteri · 2023-10-17T09:16:28Z

There is one issue: for the disabled Chip you can highlight the text.

src/Chip/README.md

src/Chip/index.tsx

src/SelectableBox/tests/SelectableBoxSet.test.jsx

src/Chip/Chip.test.jsx

adamstankiewicz

This is looking great! Left a couple comments/suggestions.

adamstankiewicz · 2023-10-20T12:47:17Z

src/Chip/ChipIcon.tsx

+    );
+  }
+
+  return <Icon src={src} className={className} />;


[curious] I'm wondering if we should be using the size="sm" attribute for the Chip icons so it more closely matches the small font size used inside the Chip?

src/Chip/ChipIcon.tsx

adamstankiewicz · 2023-10-20T12:55:44Z

src/Chip/index.tsx

+    tabIndex={0}
+    role="button"


If the left/right icons included in the Chip are interactive, I believe the intent is to not have the top-level Chip be interactive as a button, where only the icon(s) are interactive. This prevents possible usability issues where user is trying to dismiss the Chip but accidentally clicks on the parent Chip container instead of the IconButton.

Similarly, if the icon(s) are interactive, the parent Chip should remain unchanged from a hover styles perspective. The only hover focus in this case should be on the IconButton itself.

Great points! I've refactored some stuff here and updated docs and I believe I achieved the behaviour you described, so please take a look again and let me know what you think 🙂

adamstankiewicz · 2023-10-20T12:56:50Z

src/SelectableBox/tests/SelectableBoxSet.test.jsx

@@ -1,3 +1,4 @@
+/* eslint-disable no-console */


nit: is this eslint disable line still relevant?

Not after rebasing on master, thanks 🙂

src/Chip/ChipIcon.tsx

refactor: added selected state refactor: added IconButton refactor: styles refactoring refactor: added tests refactor: added new SCSS variables

adamstankiewicz

This is looking great; I added one suggestion related to alt text props and a couple minor nits. Even though none of the existing Chip props seem to have changed, I'm thinking this may technically need to be released as a breaking change due to the change in some of the SCSS theme variables that theme authors may have been relying on.

Given this Chip breaking change and the other breaking change we're considering (Pagination), perhaps we consider creating a single major version release that contains the breaking changes of both components to avoid back-to-back major version releases.

adamstankiewicz · 2023-11-06T12:52:00Z

src/Chip/README.md

+Use `iconBefore` and `iconAfter` props to provide icons for the `Chip`, note that you also have to provide
+accessible names for these icons for screen reader support via `iconBeforeAlt` and `iconAfterAlt` respectively. 


I believe the associated alt accessible name should only be required when the icon itself is clickable. If an icon is not interactive, it may mean the icon is purely decorative and may not have any semantic meaning for screen reader users. As opposed to an interactive icon (e.g., "X" to dismiss the Chip), which consumers would need to provide an alt to describe the intent of the interactive icon to screen readers.

For example, the following Chip with a person icon; the Chip contents describe the user where the icon itself could be considered decorative vs. semantic (i.e., the icon itself isn't what's describing the contents of the Chip).

@adamstankiewicz Do we need to remove the prop to add alt text for an icon that is not clickable? Or will we just edit the supporting text on the documentation site and leave this option?

@PKulkoRaccoonGang As is, I believe the alt text for the icons is always required whenever an icon is passed. I would recommend modifying the conditionally required alt text based not only on whenever an icon is passed, but also if its associated click handler is passed as well. E.g., perhaps something along the lines of:

Chip.propTypes = { iconBeforeAlt: requiredWhen(PropTypes.string, ['iconBefore', 'onIconBeforeClick']), };

@PKulkoRaccoonGang I think requiredWhen accepts only strings as a second argument and it does not know how to work with lists, so right now this prop validation does not work, can you please check this? (and modify requiredWhen if necessary)

Also, we probably need to change have to to can in the description of this example, because it's not always required to pass alt text anymore.

Thanks, corrected

src/Chip/README.md

openedx-webhooks · 2023-11-21T17:36:48Z

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

openedx-webhooks added the blended PR is managed through 2U's blended developmnt program label Oct 11, 2023

PKulkoRaccoonGang marked this pull request as draft October 11, 2023 16:27

PKulkoRaccoonGang force-pushed the Peter_Kulko/chip-redesign branch from a79c5b5 to acad78f Compare October 16, 2023 12:01

PKulkoRaccoonGang force-pushed the Peter_Kulko/chip-redesign branch from efa81af to 9fa3921 Compare October 16, 2023 12:32

PKulkoRaccoonGang marked this pull request as ready for review October 16, 2023 12:35

PKulkoRaccoonGang requested review from monteri, adamstankiewicz and viktorrusakov October 16, 2023 12:55

monteri reviewed Oct 17, 2023

View reviewed changes

src/Chip/mixins.scss Outdated Show resolved Hide resolved

src/Chip/index.tsx Outdated Show resolved Hide resolved

src/Chip/index.scss Outdated Show resolved Hide resolved

monteri reviewed Oct 17, 2023

View reviewed changes

src/Chip/index.tsx Outdated Show resolved Hide resolved

PKulkoRaccoonGang force-pushed the Peter_Kulko/chip-redesign branch 2 times, most recently from e9a9520 to 2142773 Compare October 17, 2023 13:43

PKulkoRaccoonGang requested a review from monteri October 17, 2023 15:40

PKulkoRaccoonGang force-pushed the Peter_Kulko/chip-redesign branch from 68e4598 to bf80855 Compare October 18, 2023 13:23

viktorrusakov reviewed Oct 19, 2023

View reviewed changes

src/Chip/README.md Outdated Show resolved Hide resolved

viktorrusakov reviewed Oct 19, 2023

View reviewed changes

src/Chip/index.tsx Outdated Show resolved Hide resolved

viktorrusakov reviewed Oct 19, 2023

View reviewed changes

src/Chip/index.tsx Outdated Show resolved Hide resolved

src/SelectableBox/tests/SelectableBoxSet.test.jsx Outdated Show resolved Hide resolved

src/Chip/Chip.test.jsx Outdated Show resolved Hide resolved

src/Chip/Chip.test.jsx Outdated Show resolved Hide resolved

viktorrusakov linked an issue Oct 19, 2023 that may be closed by this pull request

Chip implementation updates #2645

Closed

adamstankiewicz reviewed Oct 20, 2023

View reviewed changes

viktorrusakov reviewed Oct 26, 2023

View reviewed changes

src/Chip/ChipIcon.tsx Outdated Show resolved Hide resolved

viktorrusakov force-pushed the Peter_Kulko/chip-redesign branch from 0bd7c0f to b50147b Compare October 27, 2023 11:17

PKulkoRaccoonGang added 5 commits November 6, 2023 14:22

feat: Chip component redesign

15ccfda

refactor: added selected state refactor: added IconButton refactor: styles refactoring refactor: added tests refactor: added new SCSS variables

refactor: added translations

2b0985d

refactor: refactoring after review

bbf1d30

refactor: refactoring after review

75a3be4

fix: fixed proptypes

90e5bab

viktorrusakov and others added 2 commits November 6, 2023 14:30

fix: update interactive behaviour

245ec2a

fix: fixed tabindex for disabled variant

d6becf4

PKulkoRaccoonGang force-pushed the Peter_Kulko/chip-redesign branch from 0fe30c0 to d6becf4 Compare November 6, 2023 12:35

adamstankiewicz reviewed Nov 6, 2023

View reviewed changes

PKulkoRaccoonGang added 2 commits November 7, 2023 11:45

refactor: refactoring after review

840bf7b

refactor: refactoring after rebase

ddf4b82

PKulkoRaccoonGang marked this pull request as draft November 9, 2023 11:38

refactor: added array support for requiredWhen

8fa7b1a

PKulkoRaccoonGang force-pushed the Peter_Kulko/chip-redesign branch from da74e3d to 8fa7b1a Compare November 9, 2023 13:42

PKulkoRaccoonGang marked this pull request as ready for review November 9, 2023 13:42

adamstankiewicz mentioned this pull request Nov 21, 2023

feat: implement cancel button for pending assignment openedx/frontend-app-admin-portal#1099

Merged

3 tasks

PKulkoRaccoonGang changed the title ~~[BD-46] feat: Chip component redesign~~ [OLD] feat: Chip component redesign Nov 21, 2023

openedx-webhooks added open-source-contribution PR author is not from Axim or 2U and removed blended PR is managed through 2U's blended developmnt program labels Nov 21, 2023

PKulkoRaccoonGang mentioned this pull request Nov 21, 2023

[BD-46] feat: Chip component redesign #2836

Merged

10 tasks

PKulkoRaccoonGang closed this Nov 21, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[OLD] feat: Chip component redesign #2713

[OLD] feat: Chip component redesign #2713

PKulkoRaccoonGang commented Oct 11, 2023 •

edited

Loading

netlify bot commented Oct 11, 2023 •

edited

Loading

openedx-webhooks commented Oct 11, 2023 •

edited

Loading

codecov bot commented Oct 16, 2023 •

edited

Loading

monteri commented Oct 17, 2023

adamstankiewicz left a comment

adamstankiewicz Oct 20, 2023

adamstankiewicz Oct 20, 2023

viktorrusakov Oct 27, 2023

adamstankiewicz Oct 20, 2023

viktorrusakov Oct 27, 2023

adamstankiewicz left a comment

adamstankiewicz Nov 6, 2023

PKulkoRaccoonGang Nov 6, 2023

adamstankiewicz Nov 6, 2023 •

edited

Loading

viktorrusakov Nov 9, 2023

PKulkoRaccoonGang Nov 10, 2023

openedx-webhooks commented Nov 21, 2023

		Use `iconBefore` and `iconAfter` props to provide icons for the `Chip`, note that you also have to provide
		accessible names for these icons for screen reader support via `iconBeforeAlt` and `iconAfterAlt` respectively.

[OLD] feat: Chip component redesign #2713

[OLD] feat: Chip component redesign #2713

Conversation

PKulkoRaccoonGang commented Oct 11, 2023 • edited Loading

Description

Deploy Preview

Merge Checklist

Post-merge Checklist

netlify bot commented Oct 11, 2023 • edited Loading

✅ Deploy Preview for paragon-openedx ready!

openedx-webhooks commented Oct 11, 2023 • edited Loading

codecov bot commented Oct 16, 2023 • edited Loading

Codecov Report

monteri commented Oct 17, 2023

adamstankiewicz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adamstankiewicz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adamstankiewicz Nov 6, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

openedx-webhooks commented Nov 21, 2023

PKulkoRaccoonGang commented Oct 11, 2023 •

edited

Loading

netlify bot commented Oct 11, 2023 •

edited

Loading

openedx-webhooks commented Oct 11, 2023 •

edited

Loading

codecov bot commented Oct 16, 2023 •

edited

Loading

adamstankiewicz Nov 6, 2023 •

edited

Loading