-
Notifications
You must be signed in to change notification settings - Fork 55
Conversation
Codecov Report
@@ 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
Continue to review full report at Codecov.
|
c446718
to
d04ea6a
Compare
docs/src/examples/components/Carousel/Types/CarouselExample.tsx
Outdated
Show resolved
Hide resolved
navigation={{ | ||
'aria-label': 'people portraits', | ||
items: carouselItems.map((item, index) => ({ | ||
key: index, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
key: index, | |
key: item.id, |
key: index, | ||
'aria-label': imageAltTags[item.id], | ||
'aria-controls': item.id, | ||
icon: { name: 'stardust-circle', size: 'smallest' as SizeValue }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why aren't these default props for this icon?
* 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' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
navigationPosition?: 'bottom' | 'top' | 'left' | 'right' | |
navigationPosition?: 'below' | 'above' | 'start' | 'end' |
CHANGELOG.md
Outdated
@@ -34,6 +34,7 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm | |||
- 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)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- `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)) |
import * as React from 'react' | ||
import { Carousel, Image } from '@stardust-ui/react' | ||
|
||
const imageAltTags = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Inline these
circular?: boolean | ||
|
||
/** Initial activeIndex value. */ | ||
defaultActiveIndex?: number | string |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add onActiveIndexChange
method
navigationPosition?: 'bottom' | 'top' | 'left' | 'right' | ||
|
||
/** A Carousel's paddles may fade away when mouse is not hovering the code. */ | ||
paddlesFade?: boolean |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's not add this prop just now.. If we see need in the future we can add it.
* 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' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
|
||
static defaultProps = { | ||
accessibility: carouselBehavior, | ||
as: 'div', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is by default, no need to add it here
defaultProps: () => ({ | ||
className: Carousel.slotClassNames.paddlePrevious, | ||
iconOnly: true, | ||
icon: 'stardust-chevron-left', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's add these in the defaultProps, instead of the empty objects.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
paddlePrevious, | ||
), | ||
}), | ||
overrideProps: (predefinedProps: ButtonProps) => ({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Extract these to functions
} | ||
|
||
static slotClassNames: CarouselItemSlotClassNames = { | ||
itemPositionContainer: `${CarouselItem.className}__itemPositionContainer`, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
itemPositionText
secondary?: boolean | ||
|
||
/** Carousel navigation items can by highlighted using underline. */ | ||
underlined?: boolean |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's not add underlined for now
} | ||
|
||
handleBlur = (e: React.SyntheticEvent) => { | ||
this.setState({ isFromKeyboard: false }) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Try replacing this with 'focus-visible'
@@ -33,6 +33,8 @@ const theme: ThemeInput = { | |||
'stardust-arrow-up': processedIcons['triangle-up'], | |||
'stardust-arrow-down': processedIcons['triangle-down'], | |||
'stardust-arrow-end': processedIcons['triangle-right'], | |||
'stardust-chevron-left': processedIcons['chevron-left'], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please use start
and end
|
||
paddleNext.simulate('click') | ||
|
||
expect(pagination.getDOMNode().textContent).toBe(`4 of ${items.length}`) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please check this again, paddleNext should not be visible
jest.runAllTimers() | ||
}) | ||
|
||
it('should not show pagination if prop is passed', () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it('should not show pagination if prop is passed', () => { | |
it('should not show pagination if navigation prop is passed', () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And add the opposite scenario test (navigation: false
)
…-ui/react into feat/carousel-component
…-ui/react into feat/carousel-component
@@ -207,6 +209,8 @@ export default { | |||
'canvas-add-page': canvasAddPage, | |||
chat, | |||
'chevron-down': chevronDown, | |||
'chevron-start': chevronStart, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure if we should export these icons. Can you please just export the stardust-* icons
Separate component for the navigation (based on the Menu implementation).
Features:
After API discussion: