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.

Sign up for GitHub

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

Jump to bottom

(fix) RTL styles for Accordion #915

Merged
merged 6 commits into from
Jun 21, 2018
Merged

(fix) RTL styles for Accordion #915

merged 6 commits into from
Jun 21, 2018

Conversation

irafishbein
Copy link
Contributor

Overview

Resolves #914

I've added styles that are harmless for LTR mode and resolve issue in RTL mode.

Fixed:
image
image

Testing / Reviewing
Run Accordion on RTL screen,
Verify arrow direction when open and closed.

@irafishbein
Copy link
Contributor Author

@alisonjoseph, @tw15egan
Hello,
I was unable to set you as reviewer (probably my account issues).
Can you please take a look?
Thank you.

@irafishbein
Copy link
Contributor Author

irafishbein commented Jun 20, 2018 •
edited
Loading

@alisonjoseph, @tw15egan
Hello,
I was unable to set you as reviewers (probably my account issues).
Can you please take a look?
Thank you.

@alisonjoseph
Copy link
Member

I checked this in Chrome on a mac and it didn't seem to change the arrow direction in RTL mode.
screen shot 2018-06-20 at 7 51 06 am

@irafishbein
Copy link
Contributor Author

@alisonjoseph
Sorry, fixed, please retry.

@alisonjoseph
Copy link
Member

Hmm, am I doing something wrong, it still doesn't appear to be adding this style /*rtl:rotate(180deg)*/;

I added <body dir="rtl"> to my page, is there anything else I need to do?

@asudoh
Copy link
Contributor

asudoh commented Jun 20, 2018

Great point @alisonjoseph - Seems that @irafishbein is using http://rtlcss.com/learn/usage-guide/control-directives/index.html or a like, and if it's the case, we need a PR in https://github.com/carbon-design-system/design-system-website to add a explanation of how to generate RTL version of the CSS.

@irafishbein
Copy link
Contributor Author

@alisonjoseph
dir='rtl' should be set at level.
Please try it this way - it should work.

@irafishbein
Copy link
Contributor Author

@asudoh
You are right, I am using https://www.npmjs.com/package/webpack-rtl-plugin.
Please let me know if you want me to provide 'RTL support instructions'.

tw15egan
tw15egan approved these changes Jun 21, 2018
View reviewed changes
Copy link
Collaborator

@tw15egan tw15egan left a comment

Choose a reason for hiding this comment

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

I'm also unable to get it to work properly in RTL mode, but since the styles still look fine in LTR mode, and the other RTL fix worked great, I'm fine with approving this

@tw15egan
Copy link
Collaborator

I agree with @asudoh that we should add some documentation on the main website regarding RTL mode, but maybe after we verify all the components work properly first 😄

alisonjoseph
alisonjoseph approved these changes Jun 21, 2018
View reviewed changes
Copy link
Member

@alisonjoseph alisonjoseph left a comment

Choose a reason for hiding this comment

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

Agree with TJ

@alisonjoseph alisonjoseph merged commit a6500e4 into carbon-design-system:master Jun 21, 2018
@carbon-bot
Copy link
Contributor

🎉 This PR is included in version 9.1.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tw15egan tw15egan tw15egan approved these changes

@alisonjoseph alisonjoseph alisonjoseph approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Accordion - wrong direction of arrow in RTL mode
5 participants
@irafishbein @alisonjoseph @asudoh @tw15egan @carbon-bot