[BD-46] feat: create chip carousel #2378

monteri · 2023-06-16T11:47:54Z

Description

Create chip carousel component

Deploy Preview

https://deploy-preview-2378--paragon-openedx.netlify.app

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 · 2023-06-16T11:47:58Z

Thanks for the pull request, @monteri!

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

netlify · 2023-06-16T11:48:00Z

✅ Deploy Preview for paragon-openedx ready!

Built without sensitive environment variables

Name	Link
🔨 Latest commit	`acaff78`
🔍 Latest deploy log	https://app.netlify.com/sites/paragon-openedx/deploys/64c26a875685ce0008cb75ad
😎 Deploy Preview	https://deploy-preview-2378--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.

PKulkoRaccoonGang · 2023-06-19T15:00:29Z

src/ChipCarousel/ChipCarousel.scss

+  }
+}
+
+.pgn__chip-carousel__right-control,


These selectors are children .pgn__chip-carousel?

Yes, but if we don't need nesting, it is appropriate to avoid it to have more readable styles.

Yeah, I'd say the nesting isn't a hard requirement given we have the convention of BEM class names (mostly) prefixed with pgn__{componentname}. Unlikely for collisions to happen, though still possible.

PKulkoRaccoonGang · 2023-06-19T15:01:07Z

src/ChipCarousel/ChipCarousel.test.jsx

+import React from 'react';
+import { render, fireEvent } from '@testing-library/react';
+import { IntlProvider } from 'react-intl';
+import ChipCarousel from './index';


Suggested change

import ChipCarousel from './index';

import ChipCarousel from '.';

PKulkoRaccoonGang · 2023-06-19T15:02:13Z

src/ChipCarousel/README.md

+notes: |
+---
+
+``ChipCarousel`` component creates a scrollable horizontal block of chips with buttons for navigating to the previous and next set of chips.


Suggested change

``ChipCarousel`` component creates a scrollable horizontal block of chips with buttons for navigating to the previous and next set of chips.

The ``ChipCarousel`` component creates a scrollable horizontal block of chips with buttons for navigating to the previous and next set of chips.

PKulkoRaccoonGang · 2023-06-19T15:03:10Z

src/ChipCarousel/README.md

+  />
+```
+
+## `Chip` Carousel


Interactive Сhip Сarousel maybe

I just forgot to delete this example

PKulkoRaccoonGang · 2023-06-19T15:13:46Z

src/ChipCarousel/README.md

+        </div>
+        <div ref={setOverflowRef} className="d-flex">
+          <OverflowScroll.Items>
+            <Chip iconAfter={Close}>New</Chip>


I propose to simulate an array in this way. This will help us keep the example compact.

{[...Array(12).keys()].map(item => ( <Chip key={item + 1} iconAfter={Close}>New</Chip> ))}

Already removed code, thank you for noticing

PKulkoRaccoonGang · 2023-06-19T15:15:14Z

src/ChipCarousel/index.tsx

+import { useIntl } from 'react-intl';
+import classNames from 'classnames';
+// @ts-ignore
+import { OverflowScroll, OverflowScrollContext } from '../OverflowScroll';


Let's try to ignore the necessary import rule for the whole file, this will help to avoid numerous @ts-ignore

I would do it as a separate PR

[clarification] Once the Paragon types PR merges in, I'm assuming these @ts-ignore comment would become unnecessary?

I checked by adding d.ts file for Icon component. After that I could remove ts-ignore for import Icon ....

PKulkoRaccoonGang · 2023-06-19T15:17:35Z

src/ChipCarousel/messages.js

+
+const messages = defineMessages({
+  scrollToPrevious: {
+    id: 'pgn.ChipCarousel.scrollToPrevious',


We do not use camelСase in the declaration of transfer identifiers, let's use exclusively lowercase identifiers

I just stick to the other id in the Paragon, do you think we need to change to lowercase all of them?

id: 'pgn.chip-carousel.scroll-to-previous', maybe

We do not use camelСase in the declaration of transfer identifiers, let's use exclusively lowercase identifiers

@PKulkoRaccoonGang [clarification] Can you expand on this comment a bit more?

@PKulkoRaccoonGang @monteri After discussing, we decided to stick with the camelCase message ID format (it's used through frontend-app-learning and some other MFEs. Up to the owning team of repo to decide the standard).

PKulkoRaccoonGang · 2023-06-19T15:20:57Z

src/index.scss

@@ -11,6 +11,7 @@
 @import "./Collapsible/Collapsible";
 @import "./CloseButton/CloseButton";
 @import "./Chip/Chip";
+@import "./ChipCarousel/ChipCarousel";


Curious moment. It seems to me that if the style files are called index, we can remove the duplication of the name from the path here

For example: @import "./ChipCarousel"

Yeah, I think it can be considered as separate issue

PKulkoRaccoonGang · 2023-06-19T15:21:59Z

src/ChipCarousel/_variables.scss

@@ -0,0 +1 @@
+$chip-carousel-controls-top-offset:   -3px !default;


It will be necessary to add a new token :)

codecov · 2023-06-23T12:52:43Z

Codecov Report

Patch coverage: 85.29% and project coverage change: +0.26% 🎉

Comparison is base (319b694) 91.39% compared to head (acaff78) 91.65%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #2378      +/-   ##
==========================================
+ Coverage   91.39%   91.65%   +0.26%     
==========================================
  Files         234      236       +2     
  Lines        4161     4195      +34     
  Branches     1001     1012      +11     
==========================================
+ Hits         3803     3845      +42     
+ Misses        351      346       -5     
+ Partials        7        4       -3

Files Changed	Coverage Δ
src/OverflowScroll/OverflowScroll.jsx	`100.00% <ø> (ø)`
src/ChipCarousel/index.tsx	`84.84% <84.84%> (ø)`
src/ChipCarousel/messages.js	`100.00% <100.00%> (ø)`

... and 3 files with indirect coverage changes

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

adamstankiewicz · 2023-06-23T12:33:20Z

src/ChipCarousel/README.md

+              max: offsetType === 'percentage' ? 100 : 1000,
+              step: offsetType === 'percentage' ? 1 : 50,


On page load, offsetType is defaulted to "fixed" at a value of 120. However, if the user switches to "percentage", 120 > the max of 100, so the percentage offset changes to 100%, which doesn't quite demonstrate the percentage increment option effectively.

I wonder if we should reset the offset value when switching between offsetTypes so the offset values make sense for each type upon switching?

Added function to be more user-friendly when offsetType is switched. It calculates ration and sets relatively equal value between fixed and percentage.

adamstankiewicz · 2023-06-23T12:34:40Z

src/ChipCarousel/index.tsx

+import { useIntl } from 'react-intl';
+import classNames from 'classnames';
+// @ts-ignore
+import { OverflowScroll, OverflowScrollContext } from '../OverflowScroll';


[clarification] Once the Paragon types PR merges in, I'm assuming these @ts-ignore comment would become unnecessary?

adamstankiewicz · 2023-06-23T12:37:04Z

src/ChipCarousel/README.md

+            value: gap,
+            setValue: setGap,
+            range: { min: 0, max: 6, step: 0.5 },
+            name: 'offset'


Should this be named gap instead?

updated to gap

adamstankiewicz · 2023-06-23T12:38:55Z

src/ChipCarousel/README.md

+            value: gap,
+            setValue: setGap,
+            range: { min: 0, max: 6, step: 0.5 },
+            name: 'offset'


[curious] It looks like there's no difference in the gap between 0 and 0.5; is this expected?

Yes, probably I have talked about this case that there is no .5 spacer value.

$spacers: map-merge( ( 0: 0, 1: ($spacer * .25), 1\.5: ($spacer * .375), 2: ($spacer * .5), 2\.5: ($spacer * .75), 3: $spacer, 3\.5: ($spacer * 1.25), 4: ($spacer * 1.5), 4\.5: ($spacer * 2), 5: ($spacer * 3), 5\.5: ($spacer * 4), 6: ($spacer * 5), ), $spacers );

Ah, yes. Thank you for the reminder. Noted :)

adamstankiewicz · 2023-06-23T12:48:29Z

src/ChipCarousel/messages.js

+
+const messages = defineMessages({
+  scrollToPrevious: {
+    id: 'pgn.ChipCarousel.scrollToPrevious',


We do not use camelСase in the declaration of transfer identifiers, let's use exclusively lowercase identifiers

@PKulkoRaccoonGang [clarification] Can you expand on this comment a bit more?

adamstankiewicz · 2023-06-23T12:48:51Z

src/OverflowScroll/OverflowScroll.jsx

@@ -84,7 +84,9 @@ OverflowScroll.propTypes = {
  onScrollPrevious: PropTypes.func,
  /** Callback function for when the user scrolls to the next element. */
  onScrollNext: PropTypes.func,
-  offset: PropTypes.oneOfType([PropTypes.string, PropTypes.number]),
+  /** A value specifying the distance the scroll should move. */


adamstankiewicz · 2023-06-23T12:52:29Z

src/ChipCarousel/ChipCarousel.test.jsx

+  it('should render the carousel correctly', () => {
+    render(<TestingChipCarousel />);
+
+    const carousel = document.getElementsByClassName('pgn__chip-carousel');


Is there a way around needing to rely on the implementation details of the component in these tests by relying more on the @testing-library/react (RTL) API?

This is kind of using RTL almost as if it were Enzyme by relying on implementation details like class names. Perhaps we rely more on data-testid attributes here for selections?

function TestingChipCarousel(props) { return ( <IntlProvider locale="en"> <ChipCarousel data-testid="chip-carousel" ariaLabel={ariaLabel} items={items} {...props} /> </IntlProvider> ); } render(<TestingChipCarousel />); const carousel = screen.getByTestId('chip-carousel');

Similarly for the chipItems below, we could do something like add a data-testid="chip-component" on each child object passed to ChipCarousel such that we could query using RTL like: screen.queryByTestId('chip-component')

Reference: https://kentcdodds.com/blog/testing-implementation-details

Agree on this point. Refactored tests to use data-testid

adamstankiewicz · 2023-06-23T12:54:22Z

src/ChipCarousel/ChipCarousel.test.jsx

+
+    const chipItems = document.getElementsByClassName('pgn__chip');
+    for (let i = 0; i < chipItems.length; i++) {
+      fireEvent.click(chipItems[i]);


Typically, userEvent.click is preferred over using fireEvent directly. See https://stackoverflow.com/a/71031849.

updated to use userEvent.click

adamstankiewicz

LGTM! Just looks like it might need a rebase with master, though.

adamstankiewicz · 2023-07-21T12:41:45Z

src/ChipCarousel/ChipCarousel.scss

+  }
+}
+
+.pgn__chip-carousel__right-control,


Yeah, I'd say the nesting isn't a hard requirement given we have the convention of BEM class names (mostly) prefixed with pgn__{componentname}. Unlikely for collisions to happen, though still possible.

monteri · 2023-07-27T13:03:24Z

Yeah, I'd say the nesting isn't a hard requirement given we have the convention of BEM class names (mostly) prefixed with pgn__{componentname}. Unlikely for collisions to happen, though still possible.

Updated to use nesting in this case. Also noticed that when we changed componentName.scss to index.scss we forgot to update component-generator. Made a quick fix as part of this PR

adamstankiewicz · 2023-08-04T11:58:27Z

Updated to use nesting in this case. Also noticed that when we changed componentName.scss to index.scss we forgot to update component-generator. Made a quick fix as part of this PR

Good catch!

openedx-webhooks · 2023-08-04T12:16:56Z

@monteri 🎉 Your pull request was merged! Please take a moment to answer a two question survey so we can improve your experience in the future.

edx-semantic-release · 2023-08-04T12:33:39Z

🎉 This PR is included in version 20.46.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

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

monteri self-assigned this Jun 16, 2023

monteri requested a review from adamstankiewicz June 16, 2023 11:48

monteri marked this pull request as draft June 16, 2023 11:48

monteri mentioned this pull request Jun 16, 2023

Add ChipCarousel component #1951

Closed

monteri marked this pull request as ready for review June 19, 2023 14:58

PKulkoRaccoonGang reviewed Jun 19, 2023

View reviewed changes

adamstankiewicz reviewed Jun 23, 2023

View reviewed changes

monteri added the raccoon-gang label Jul 17, 2023

adamstankiewicz linked an issue Jul 21, 2023 that may be closed by this pull request

Add ChipCarousel component #1951

Closed

adamstankiewicz approved these changes Jul 21, 2023

View reviewed changes

monteri added 6 commits July 27, 2023 15:49

feat: create chip carousel

e6e8b46

feat: update ChipCarousel

585430f

feat: add gap and docs

b34f52a

feat: update according to PR review

2b33591

feat: PR comment update

06cc767

refactor: clean up after rebase and component-generator

acaff78

monteri force-pushed the zadorozhnii/chip-carousel branch from 045ff08 to acaff78 Compare July 27, 2023 13:00

adamstankiewicz approved these changes Aug 4, 2023

View reviewed changes

adamstankiewicz merged commit c1ecb71 into openedx:master Aug 4, 2023
8 of 9 checks passed

edx-semantic-release added the released label Aug 4, 2023

	import ChipCarousel from './index';
	import ChipCarousel from '.';

	``ChipCarousel`` component creates a scrollable horizontal block of chips with buttons for navigating to the previous and next set of chips.
	The ``ChipCarousel`` component creates a scrollable horizontal block of chips with buttons for navigating to the previous and next set of chips.

		@@ -0,0 +1 @@
		$chip-carousel-controls-top-offset: -3px !default;

		max: offsetType === 'percentage' ? 100 : 1000,
		step: offsetType === 'percentage' ? 1 : 50,

[BD-46] feat: create chip carousel #2378

[BD-46] feat: create chip carousel #2378

Conversation

monteri commented Jun 16, 2023 • edited Loading

Description

Deploy Preview

Merge Checklist

Post-merge Checklist

openedx-webhooks commented Jun 16, 2023 • edited Loading

netlify bot commented Jun 16, 2023 • edited Loading

✅ Deploy Preview for paragon-openedx ready!

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

monteri Jun 21, 2023 • edited Loading

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

Choose a reason for hiding this comment

PKulkoRaccoonGang Jun 21, 2023 • edited Loading

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

codecov bot commented Jun 23, 2023 • edited Loading

Codecov Report

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

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

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

monteri commented Jul 27, 2023 • edited Loading

adamstankiewicz commented Aug 4, 2023

openedx-webhooks commented Aug 4, 2023

edx-semantic-release commented Aug 4, 2023

monteri commented Jun 16, 2023 •

edited

Loading

openedx-webhooks commented Jun 16, 2023 •

edited

Loading

netlify bot commented Jun 16, 2023 •

edited

Loading

monteri Jun 21, 2023 •

edited

Loading

PKulkoRaccoonGang Jun 21, 2023 •

edited

Loading

codecov bot commented Jun 23, 2023 •

edited

Loading

monteri commented Jul 27, 2023 •

edited

Loading