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

Add carousel pattern #957

Merged
merged 15 commits into from
Jan 15, 2019
Merged

Add carousel pattern #957

merged 15 commits into from
Jan 15, 2019

Conversation

mcking65
Copy link
Contributor

@mcking65 mcking65 commented Dec 16, 2018

Adds a design pattern section for carousels to resolve issue #43.

Preview Pattern in Feature Branch

Carousel pattern in issue43-add-carousel-pattern branch


Preview | Diff

@mcking65 mcking65 added documentation Pattern Page Related to a page documenting a Pattern labels Dec 16, 2018
@mcking65 mcking65 added this to the 1.1 APG Release 3 milestone Dec 16, 2018
@mcking65 mcking65 self-assigned this Dec 16, 2018
Copy link
Contributor

@charmarkk charmarkk left a comment

Choose a reason for hiding this comment

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

Content looks good to me @mcking65.

Minor editorial changes, in order of appearance:
"...image carousels where each slide contains nothing more than a single image are common."
-Add "most common" instead of "common."

"Basic: Has rotation , previous slide,..."
-Remove space after rotation, "Has rotation, ..."

(Last bullet under Basic Carousel Elements) "... does not contain the word "slide".
-End punctuation goes inside of quotation marks.

(Under Grouped Carousel Elements) "Choose slide to display".
-End punctuation goes inside of quotation marks.

@shirsha
Copy link

shirsha commented Dec 17, 2018

I like the updated definition but the word "horizontal scrolling" is misleading as carousels do not use a scroll bar to navigate through the slides.

@mcking65
Copy link
Contributor Author

@charmarkk, many, many thanks for the review!!!

"...image carousels where each slide contains nothing more than a single image are common."
-Add "most common" instead of "common."

Do we know this for sure? Does adding "most" enhance the meaning? If they are most common, extremely common, or just very common, they are nonetheless common. I'm happy to add "most" if we really know it to be the case, and it meaningfully strengthens the description.

"Basic: Has rotation , previous slide,..."
-Remove space after rotation, "Has rotation, ..."

Fixed in commit 8856024.

(Last bullet under Basic Carousel Elements) "... does not contain the word "slide".
-End punctuation goes inside of quotation marks.

Fixed in commit 8856024.

(Under Grouped Carousel Elements) "Choose slide to display".
-End punctuation goes inside of quotation marks.

Fixed in commit 8856024.

BTW, when doing a review, you can put your comments right on the line of the file that needs to be changed. Go to the files tab and click the add comment button by the line the needs changing. That is one way to start a review. You can glob a bunch of comments like that together into a review.

@mcking65
Copy link
Contributor Author

mcking65 commented Dec 18, 2018

@shirsha commented:

I like the updated definition but the word "horizontal scrolling" is misleading as carousels do not use a scroll bar to navigate through the slides.

Thank you Siri. Removed term horizontal scrolling. Please have a look at the latest.

@mcking65
Copy link
Contributor Author

mcking65 commented Dec 18, 2018

@jongund, @annabbott, @MichielBijl, I have updated the branch to reflect all feedback from the meeting and Mark's feedback as well. This is now ready for a hopefully final round of review.

@charmarkk
Copy link
Contributor

@mcking65 TIL, my thanks!

As for the question of adding “most” or not, I mostly mentioned that because it read better. If it’s a question of veracity, however, I’m happy leaving it as is.

@jongund jongund requested review from jongund and removed request for jongund January 7, 2019 19:52
@jongund
Copy link
Contributor

jongund commented Jan 7, 2019

@mcking65 The carousel design pattern looks good to me. I have updated the carousel example to include aria-roledescription on the container section element.

aria-practices.html Outdated Show resolved Hide resolved
@annabbott
Copy link

One review change noted in file. Otherwise looks good.

@mcking65 mcking65 merged commit ca78f0a into master Jan 15, 2019
@mcking65 mcking65 deleted the issue43-add-carousel-pattern branch January 15, 2019 02:12
michael-n-cooper pushed a commit that referenced this pull request Jan 15, 2019
Add carousel pattern (pull #957)

Completes drafting of the carousel design pattern for issue #43.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Pattern Page Related to a page documenting a Pattern
Development

Successfully merging this pull request may close these issues.

5 participants