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

Updated Carousel and ImageSet spec #8220

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

jwoo-msft
Copy link
Member

@jwoo-msft jwoo-msft commented Jan 9, 2023

Related Issue

N/A

Description

Updated Carousel and ImageSet design discussion docs for v-team spec review.

Sample Card

N/A

How Verified

N/A

Microsoft Reviewers: Open in CodeFlow

## Timer Property
* `"timer" : number`
* when set, the number sets the duration before a Carousel page transitions to the next Carousel page.
* while hovering, mouse click, and touch events, this auto transition is canceled.
Copy link
Contributor

Choose a reason for hiding this comment

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

Will the timer start again after the manual transition has been completed?


This will enable them to show a chat as a group chat in a messenger app.
#### Key Features
* Stack a maximum of 2 images. The green status indicator would not be displayed.
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a reason for displaying only 2 images?
3 and more could be displayed in a slightly different fashion (similar to Teams?)

Copy link
Member Author

Choose a reason for hiding this comment

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

No, this is client specific feature. we are going to share this spec with Team to expand the spec further.

So here we have a sample carousel card. You'll probably notice the ominously-named `TBD` property right away -- we haven't come up with a name for it yet. At any rate, this property is an object containing a `type` property describing what type of card this is. Other properties in this object can be provided to configure behaviors of this type of card (in this example, the timer to use for automatically flipping through the items in a carousel).

You'll probably also notice the new `CarouselPage` element. It acts pretty similarly to a `Container`, but with some differences:
So here we have a sample carousel card. You'll probably notice the new `CarouselPage` element. It acts pretty similarly to a `Container`, but with some differences:
Copy link
Contributor

Choose a reason for hiding this comment

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

You'll probably notice

suggestion: It's relevant to notice

* No `style` (or at least, we don't have a reason to have `style` here yet)
* No `bleed`
* We don't allow every element type within a `CarouselPage`, only the subset we decide on (see following sections for more details)

Having this new element also has the advantage of allowing us to reuse it later should we decide to promote the idea of `Carousel` as a regular page element (a `Carousel` would be a collection of `CarouselPage`s in this regime, though we'd need to decide how to reconcile `TBD` against per-`Carousel` settings).
Having this new element also has the advantage of allowing us to reuse it later should we decide to promote the idea of `Carousel` as a regular page element (a `Carousel` would be a collection of `CarouselPage`s in this regime.
Copy link
Contributor

Choose a reason for hiding this comment

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

missing closing ')'

* No `style` (or at least, we don't have a reason to have `style` here yet)
* No `bleed`
* We don't allow every element type within a `CarouselPage`, only the subset we decide on (see following sections for more details)

Having this new element also has the advantage of allowing us to reuse it later should we decide to promote the idea of `Carousel` as a regular page element (a `Carousel` would be a collection of `CarouselPage`s in this regime, though we'd need to decide how to reconcile `TBD` against per-`Carousel` settings).
Having this new element also has the advantage of allowing us to reuse it later should we decide to promote the idea of `Carousel` as a regular page element (a `Carousel` would be a collection of `CarouselPage`s in this regime.
Copy link
Contributor

Choose a reason for hiding this comment

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

would be a collection of CarouselPages

suggestion: is a collection of 'Carousel Page's


This will enable them to show a chat as a group chat in a messenger app.
#### Key Features
* Stack a maximum of 2 images. The green status indicator would not be displayed.
Copy link
Contributor

Choose a reason for hiding this comment

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

Change for spaces

* `style`: "stacked"
* indicates that images are displayed in a stack
* `offset`: a number
* indicates the magnitude of offset in pixels. This is how far apart diagonally the images are at. Images are at tangent at 0. In other word, there will be no space between the images except the border. Negative values moves images closer up to the image's diameter. Positive values move images farther and there is no limitation imposed by the spec, but will result in bad UI with extreme value.
Copy link
Contributor

Choose a reason for hiding this comment

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

is there some way to specify size of the stacked on top of the other image or is the stacked image always a certain size?

|Capability|Priority|
|---|---|
|Allow users to stack at least 2 images |p0
|The stacked image sets support "style": "person" so they are rounded|p0
Copy link
Contributor

Choose a reason for hiding this comment

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

extra tab

Copy link
Member Author

Choose a reason for hiding this comment

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

sorry, I don't see the extra tab.

|---|---|
|Allow users to stack at least 2 images |p0
|The stacked image sets support "style": "person" so they are rounded|p0
|The stacked images stack diagonally with the front image on the left |p0
Copy link
Contributor

Choose a reason for hiding this comment

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

terminate line with |

@ghost ghost added the no-recent-activity label Jan 15, 2023
@ghost
Copy link

ghost commented Jan 15, 2023

Hi @jwoo-msft. This pull request has had no recent activity for the past 5 days . Please take the necessary actions (review, address feedback or commit if reviewed already) to move this along.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants