feat(Carousel): base component by silviuaavram · Pull Request #1979 · microsoft/fluent-ui-react

silviuaavram · 2019-09-27T12:07:23Z

Separate component for the navigation (based on the Menu implementation).

Features:

After API discussion:

have component for Navigation, paddles.
have content rendered in item instead of Slide.

codecov · 2019-09-27T12:22:06Z

Codecov Report

Merging #1979 into master will decrease coverage by 0.14%.
The diff coverage is 3.79%.

@@            Coverage Diff             @@
##           master    #1979      +/-   ##
==========================================
- Coverage   75.83%   75.68%   -0.15%     
==========================================
  Files         160      163       +3     
  Lines        5569     5572       +3     
  Branches     1628     1620       -8     
==========================================
- Hits         4223     4217       -6     
- Misses       1332     1342      +10     
+ Partials       14       13       -1

Impacted Files	Coverage Δ
packages/react/src/index.ts	`100% <ø> (ø)`	⬆️
...ackages/react/src/components/Carousel/Carousel.tsx	`1.51% <1.51%> (ø)`
...ges/react/src/components/Carousel/CarouselItem.tsx	`12.5% <12.5%> (ø)`
...es/react/src/components/Carousel/CarouselSlide.tsx	`20% <20%> (ø)`
...ckages/react/src/components/Popup/PopupContent.tsx	`95.45% <0%> (-4.55%)`	⬇️
...ges/react/src/components/Attachment/Attachment.tsx	`87.5% <0%> (-3.81%)`	⬇️
...kages/react/src/components/Toolbar/ToolbarItem.tsx	`45.16% <0%> (ø)`	⬆️
packages/react/src/lib/renderComponent.tsx	`84.05% <0%> (ø)`	⬆️
...es/react/src/components/Tooltip/TooltipContent.tsx	`100% <0%> (ø)`	⬆️
... and 3 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d4cc17b...0977bb6. Read the comment docs.

packages/react/src/components/Carousel/Carousel.tsx

packages/react/src/components/Carousel/CarouselItem.tsx

docs/src/examples/components/Carousel/Types/CarouselExample.tsx

mnajdova · 2019-11-14T13:29:09Z

docs/src/examples/components/Carousel/Types/CarouselExample.tsx

+    navigation={{
+      'aria-label': 'people portraits',
+      items: carouselItems.map((item, index) => ({
+        key: index,


Suggested change

key: index,

key: item.id,

mnajdova · 2019-11-14T13:30:03Z

docs/src/examples/components/Carousel/Types/CarouselExample.tsx

+        key: index,
+        'aria-label': imageAltTags[item.id],
+        'aria-controls': item.id,
+        icon: { name: 'stardust-circle', size: 'smallest' as SizeValue },


Why aren't these default props for this icon?

mnajdova · 2019-11-14T13:38:02Z

packages/react/src/components/Carousel/Carousel.tsx

+   * A Carousel can position its navigation to the bottom by default,
+   * to the top, to the left or to the right of the content.
+   */
+  navigationPosition?: 'bottom' | 'top' | 'left' | 'right'


mnajdova · 2019-11-15T10:15:57Z

CHANGELOG.md

 - Add bounce animation to button clicks in Teams theme @notandrew ([#1724](https://github.com/stardust-ui/react/pull/1724))
 - Update Silver color scheme, adding `foregroundHover`, `foregroundPressed` and `background` definitions @pompomon ([#2078](https://github.com/microsoft/fluent-ui-react/pull/2078))
 - Expanding experimental accessibility schema to more components @mshoho ([#2052](https://github.com/stardust-ui/react/pull/2052))
+- `Carousel` component @silviuavram ([#1979](https://github.com/microsoft/fluent-ui-react/pull/1979))


Suggested change

- `Carousel` component @silviuavram ([#1979](https://github.com/microsoft/fluent-ui-react/pull/1979))

- Add base `Carousel` component @silviuavram ([#1979](https://github.com/microsoft/fluent-ui-react/pull/1979))

mnajdova · 2019-11-15T10:20:41Z

docs/src/examples/components/Carousel/Types/CarouselPaginationExample.tsx

+import * as React from 'react'
+import { Carousel, Image } from '@stardust-ui/react'
+
+const imageAltTags = {


Inline these

mnajdova · 2019-11-15T10:27:06Z

packages/react/src/components/Carousel/Carousel.tsx

+  circular?: boolean
+
+  /** Initial activeIndex value. */
+  defaultActiveIndex?: number | string


add onActiveIndexChange method

mnajdova · 2019-11-15T10:31:15Z

packages/react/src/components/Carousel/Carousel.tsx

+  navigationPosition?: 'bottom' | 'top' | 'left' | 'right'
+
+  /** A Carousel's paddles may fade away when mouse is not hovering the code. */
+  paddlesFade?: boolean


Let's not add this prop just now.. If we see need in the future we can add it.

mnajdova · 2019-11-15T10:32:52Z

packages/react/src/components/Carousel/Carousel.tsx

+   * On slide transition, the Carousel may translate the slides' position,
+   * fade their appearance or just hide and show without any animation.
+   */
+  slideTransition?: 'translate' | 'fade' | 'display'


Let's not add this at this moment, and implement in the styles whatever is necessary for the team. I don't think one application will have different slideTransition. If this turns out to be a case, we can add it later.

mnajdova · 2019-11-15T10:34:55Z

packages/react/src/components/Carousel/Carousel.tsx

+
+  static defaultProps = {
+    accessibility: carouselBehavior,
+    as: 'div',


this is by default, no need to add it here

mnajdova · 2019-11-15T10:46:23Z

packages/react/src/components/Carousel/Carousel.tsx

+            defaultProps: () => ({
+              className: Carousel.slotClassNames.paddlePrevious,
+              iconOnly: true,
+              icon: 'stardust-chevron-left',


let's add these in the defaultProps, instead of the empty objects.

If I do this, it will override all of them each time I add only one field. For instance in the pagination example I added aria-label as shortcuts and the default values were removed. I would consider leaving theme here.

mnajdova · 2019-11-15T10:46:58Z

packages/react/src/components/Carousel/Carousel.tsx

+                paddlePrevious,
+              ),
+            }),
+            overrideProps: (predefinedProps: ButtonProps) => ({


Extract these to functions

mnajdova · 2019-11-15T10:51:35Z

packages/react/src/components/Carousel/CarouselItem.tsx

+  }
+
+  static slotClassNames: CarouselItemSlotClassNames = {
+    itemPositionContainer: `${CarouselItem.className}__itemPositionContainer`,


itemPositionText

mnajdova · 2019-11-15T10:54:23Z

packages/react/src/components/Carousel/CarouselNavigation.tsx

+  secondary?: boolean
+
+  /** Carousel navigation items can by highlighted using underline. */
+  underlined?: boolean


Let's not add underlined for now

mnajdova · 2019-11-15T10:59:13Z

packages/react/src/components/Carousel/CarouselNavigationItem.tsx

+  }
+
+  handleBlur = (e: React.SyntheticEvent) => {
+    this.setState({ isFromKeyboard: false })


Try replacing this with 'focus-visible'

mnajdova · 2019-11-15T11:03:42Z

packages/react/src/themes/teams/withProcessedIcons.ts

    'stardust-arrow-up': processedIcons['triangle-up'],
    'stardust-arrow-down': processedIcons['triangle-down'],
    'stardust-arrow-end': processedIcons['triangle-right'],
+    'stardust-chevron-left': processedIcons['chevron-left'],


Please use start and end

mnajdova · 2019-11-15T11:06:51Z

packages/react/test/specs/components/Carousel/Carousel-test.tsx

+
+      paddleNext.simulate('click')
+
+      expect(pagination.getDOMNode().textContent).toBe(`4 of ${items.length}`)


Please check this again, paddleNext should not be visible

mnajdova · 2019-11-15T11:09:27Z

packages/react/test/specs/components/Carousel/Carousel-test.tsx

+      jest.runAllTimers()
+    })
+
+    it('should not show pagination if prop is passed', () => {


Suggested change

it('should not show pagination if prop is passed', () => {

it('should not show pagination if navigation prop is passed', () => {

And add the opposite scenario test (navigation: false)

…-ui/react into feat/carousel-component

mnajdova · 2019-11-19T09:28:40Z

packages/react/src/themes/teams/components/Icon/svg/icons/index.ts

  'canvas-add-page': canvasAddPage,
  chat,
  'chevron-down': chevronDown,
+  'chevron-start': chevronStart,


Not sure if we should export these icons. Can you please just export the stardust-* icons

silviuaavram requested review from dvdzkwsk, kuzhelov, layershifter, levithomason, miroslavstastny and mnajdova as code owners September 27, 2019 12:07

silviuaavram added new component Component planned to be added to Stardust ⚙️ enhancement New feature or request 🚧 WIP labels Sep 27, 2019

vercel bot deployed to staging September 27, 2019 12:29 View deployment

vercel bot deployed to staging September 27, 2019 12:46 View deployment

mnajdova reviewed Sep 27, 2019

View reviewed changes

packages/react/src/components/Carousel/Carousel.tsx Outdated Show resolved Hide resolved

mnajdova reviewed Sep 27, 2019

View reviewed changes

packages/react/src/components/Carousel/Carousel.tsx Outdated Show resolved Hide resolved

mnajdova reviewed Sep 27, 2019

View reviewed changes

packages/react/src/components/Carousel/Carousel.tsx Outdated Show resolved Hide resolved

mnajdova reviewed Sep 27, 2019

View reviewed changes

packages/react/src/components/Carousel/Carousel.tsx Outdated Show resolved Hide resolved

silviuaavram force-pushed the feat/carousel-component branch from c446718 to d04ea6a Compare October 1, 2019 08:55

vercel bot deployed to staging October 1, 2019 08:56 View deployment

vercel bot deployed to staging October 1, 2019 11:45 View deployment

vercel bot temporarily deployed to staging October 1, 2019 13:24 Inactive

vercel bot requested a deployment to staging October 1, 2019 13:27 Pending

vercel bot requested a deployment to staging October 1, 2019 13:30 Pending

vercel bot requested a deployment to staging October 1, 2019 13:32 Pending

vercel bot had a problem deploying to staging October 1, 2019 13:33 Failure

jurokapsiar reviewed Oct 1, 2019

View reviewed changes

packages/react/src/components/Carousel/CarouselItem.tsx Outdated Show resolved Hide resolved

jurokapsiar reviewed Oct 1, 2019

View reviewed changes

docs/src/examples/components/Carousel/Types/CarouselExample.tsx Outdated Show resolved Hide resolved

vercel bot deployed to staging October 2, 2019 08:30 View deployment

vercel bot deployed to staging October 2, 2019 09:19 View deployment

vercel bot deployed to staging October 2, 2019 12:04 View deployment

vercel bot deployed to staging October 2, 2019 12:43 View deployment

mnajdova reviewed Nov 14, 2019

View reviewed changes

Merge branch 'master' into feat/carousel-component

481a1cf

mnajdova changed the title ~~feat: carousel component~~ feat(Carousel): base component Nov 15, 2019

mnajdova reviewed Nov 15, 2019

View reviewed changes

silviuaavram added 6 commits November 18, 2019 18:19

adress code review

c3afa1e

Merge branch 'feat/carousel-component' of https://github.com/stardust…

a02e5db

…-ui/react into feat/carousel-component

fix accessibility

723e4e2

fix import

2a28b31

Merge branch 'master' into feat/carousel-component

5d429c8

Merge branch 'feat/carousel-component' of https://github.com/stardust…

6d6508b

…-ui/react into feat/carousel-component

mnajdova reviewed Nov 19, 2019

View reviewed changes

properly add icons

bb1736d

mnajdova approved these changes Nov 19, 2019

View reviewed changes

silviuaavram merged commit d88c977 into master Nov 19, 2019

silviuaavram deleted the feat/carousel-component branch November 19, 2019 10:57

	navigationPosition?: 'bottom' \| 'top' \| 'left' \| 'right'
	navigationPosition?: 'below' \| 'above' \| 'start' \| 'end'

	- `Carousel` component @silviuavram ([#1979](https://github.com/microsoft/fluent-ui-react/pull/1979))
	- Add base `Carousel` component @silviuavram ([#1979](https://github.com/microsoft/fluent-ui-react/pull/1979))


		paddleNext.simulate('click')

		expect(pagination.getDOMNode().textContent).toBe(`4 of ${items.length}`)

	it('should not show pagination if prop is passed', () => {
	it('should not show pagination if navigation prop is passed', () => {

Conversation

silviuaavram commented Sep 27, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Sep 27, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

silviuaavram commented Sep 27, 2019 •

edited

Loading

codecov bot commented Sep 27, 2019 •

edited

Loading