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

THEMES-1221: Update carousel styles #227

Merged
merged 8 commits into from
Dec 14, 2023

Conversation

nschubach
Copy link
Contributor

@nschubach nschubach commented Dec 8, 2023

Ticket

Description

Update carousel logical properties

Acceptance Criteria

The above logical properties are applied and tested on the gallery component.
Storybook is updated with RTL.

Test Steps

  1. Checkout branch - git checkout themes-1221
  2. Update dependencies - npm i

Author Checklist

  • Confirmed all the test steps a reviewer will follow above are working.
  • Ran this code locally and checked that there are not any unintended side effects. For example, that a CSS selector is scoped only to a particular block.
  • Confirmed relevant documentation has been updated/added.
  • Add label - ready for review when the pull request is ready for someone to begin reviewing

Reviewer Checklist

The reviewer of the PR should copy-paste this template into the review comments on review.

  • All GitHub Actions pass
  • Ran the code locally based on the test instructions.
  • Checked Chromatic for Storybook changes, accepted the updates if acceptable
  • Looked to see that the new or changed code has code coverage, specifically. We want the global code coverage to keep on going up with targeted testing.
  • Approve and Add label - ready to merge if you are happy with the pull request
  • Want another reviewer? Add the label additional review

@nschubach nschubach added the ready for review The PR author has completed the PR template and is ready for a review label Dec 8, 2023
@nschubach nschubach requested a review from a team as a code owner December 8, 2023 22:17
Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Remaining comments which cannot be posted as a review comment to avoid GitHub Rate Limit

stylelint

src/components/carousel/_index.scss|109 col 2| Unexpected browser feature "flexbox-gap" is not supported by KaiOS Browser 2.5 (plugin/no-unsupported-browser-features) plugin/no-unsupported-browser-features
src/components/carousel/_index.scss|118 col 3| Unexpected browser feature "css-nesting" is not supported by Opera Mobile 73, UC Browser for Android 15.5, Samsung Internet 22,23, QQ Browser 13.1, KaiOS Browser 2.5 (plugin/no-unsupported-browser-features) plugin/no-unsupported-browser-features
src/components/carousel/_index.scss|121 col 2| Unexpected browser feature "css-nesting" is not supported by Opera Mobile 73, UC Browser for Android 15.5, Samsung Internet 22,23, QQ Browser 13.1, KaiOS Browser 2.5 (plugin/no-unsupported-browser-features) plugin/no-unsupported-browser-features
src/components/carousel/_index.scss|146 col 2| Unexpected browser feature "css-nesting" is not supported by Opera Mobile 73, UC Browser for Android 15.5, Samsung Internet 22,23, QQ Browser 13.1, KaiOS Browser 2.5 (plugin/no-unsupported-browser-features) plugin/no-unsupported-browser-features
src/components/carousel/_index.scss|150 col 3| Unexpected browser feature "css-nesting" is not supported by Opera Mobile 73, UC Browser for Android 15.5, Samsung Internet 22,23, QQ Browser 13.1, KaiOS Browser 2.5 (plugin/no-unsupported-browser-features) plugin/no-unsupported-browser-features
src/components/carousel/_index.scss|194 col 2| Unexpected browser feature "css-nesting" is not supported by Opera Mobile 73, UC Browser for Android 15.5, Samsung Internet 22,23, QQ Browser 13.1, KaiOS Browser 2.5 (plugin/no-unsupported-browser-features) plugin/no-unsupported-browser-features
src/components/carousel/_index.scss|196 col 3| Unexpected browser feature "css-nesting" is not supported by Opera Mobile 73, UC Browser for Android 15.5, Samsung Internet 22,23, QQ Browser 13.1, KaiOS Browser 2.5 (plugin/no-unsupported-browser-features) plugin/no-unsupported-browser-features
src/components/carousel/_index.scss|198 col 2| Unexpected browser feature "css-nesting" is not supported by Opera Mobile 73, UC Browser for Android 15.5, Samsung Internet 22,23, QQ Browser 13.1, KaiOS Browser 2.5 (plugin/no-unsupported-browser-features) plugin/no-unsupported-browser-features
src/components/carousel/_index.scss|200 col 3| Unexpected browser feature "css-nesting" is not supported by Opera Mobile 73, UC Browser for Android 15.5, Samsung Internet 22,23, QQ Browser 13.1, KaiOS Browser 2.5 (plugin/no-unsupported-browser-features) plugin/no-unsupported-browser-features
src/components/carousel/_index.scss|198 col 2| Expected empty line before rule (rule-empty-line-before) rule-empty-line-before
src/components/carousel/_index.scss|193 col 1| Unexpected qualifying type selector "html[dir="rtl"]" (selector-no-qualifying-type) selector-no-qualifying-type
src/components/carousel/_index.scss|194 col 2| Unexpected qualifying type selector "html[dir="rtl"]" (selector-no-qualifying-type) selector-no-qualifying-type
src/components/carousel/_index.scss|198 col 2| Unexpected qualifying type selector "html[dir="rtl"]" (selector-no-qualifying-type) selector-no-qualifying-type

@vgalatro vgalatro self-assigned this Dec 12, 2023
@vgalatro vgalatro added review in progress A review is underway. Even if an approval has been submitted, wait for all reviews to be completed. and removed ready for review The PR author has completed the PR template and is ready for a review labels Dec 12, 2023
Copy link
Contributor

@vgalatro vgalatro left a comment

Choose a reason for hiding this comment

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

The changes look good, just noticed some duplicate properties between the _index.scss and the news.json. The changes are also missing from the commerce.json.

Could you add a RTL story to index.stories.mdx as well? So we have it whenever we update Storybook.

Comment on lines 7 to 11
container-name: c-carousel;
container-type: inline-size;
display: grid;
grid-template-areas: "controls" "carousel" "indicators";
max-inline-size: 100cqi;
Copy link
Contributor

Choose a reason for hiding this comment

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

Are these three props (container-name, container-type, and max-inline-size) supposed to be here? They're also on the token, so they end up appearing twice in the final CSS.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nope, I removed these. I missed moving these over.

@vgalatro vgalatro added needs changes The reviewer has requested changes from the PR author and removed review in progress A review is underway. Even if an approval has been submitted, wait for all reviews to be completed. labels Dec 12, 2023
@nschubach nschubach added ready for review The PR author has completed the PR template and is ready for a review and removed needs changes The reviewer has requested changes from the PR author labels Dec 13, 2023
@nschubach nschubach requested a review from vgalatro December 13, 2023 23:20
Copy link
Contributor

@vgalatro vgalatro left a comment

Choose a reason for hiding this comment

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

Changes look good, approved!

@vgalatro vgalatro added ready to merge It's time! Merge this PR. Woo! and removed ready for review The PR author has completed the PR template and is ready for a review labels Dec 14, 2023
@nschubach nschubach merged commit e26da73 into arc-themes-release-version-2.1.3 Dec 14, 2023
@nschubach nschubach deleted the themes-1221 branch December 14, 2023 18:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready to merge It's time! Merge this PR. Woo!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants