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

[WIP]style(beta): use logical properties #613

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

321gillian
Copy link
Collaborator

@321gillian 321gillian commented Sep 23, 2024

This is just kind of a POC for now until I get a bit of testing done with a native user of rtl. Don't panic when you see all the additions.

✅ Closes: COMUI-3096

Copy link

@daragh-king-genesys
Copy link
Collaborator

I would have expected to see a change here and then running the linter sort out the actual scss files

@321gillian 321gillian self-assigned this Sep 24, 2024
@321gillian 321gillian changed the title feat(action-button): add rtl support style(all): use logical properties Sep 24, 2024
@321gillian 321gillian changed the title style(all): use logical properties [WIP]style(all): use logical properties Sep 24, 2024
@321gillian
Copy link
Collaborator Author

Some issues here with popovers and the toggle. Looking into them.

Comment on lines 39 to 41
// TODO: Use :dir() selector instead when Chrome improves support for it
const compStyles = window.getComputedStyle(this.checkboxElement);
this.direction = compStyles.direction;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

flagging manual changes in this file

"plugins": ["stylelint-use-logical-spec"],
"rules": {
"liberty/use-logical-spec": true
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Looking at this PR again I think we should do this in a few PRs over a few weeks to reduce the risk. We are also not going to want to change the legacy components at all so we will need to have an override for that.

See can you find a config that changes about 40 files. Also have the example.html files changes in their own PR as they are not that critical.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Was thinking it might be good to start with beta components and go from there.

@321gillian 321gillian changed the title [WIP]style(all): use logical properties style(beta): use logical properties Oct 9, 2024
@321gillian 321gillian changed the title style(beta): use logical properties [WIP]style(beta): use logical properties Oct 9, 2024
Copy link
Collaborator

@katie-bobbe-genesys katie-bobbe-genesys left a comment

Choose a reason for hiding this comment

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

I took a look at the beta components in different browsers and I did not see any regressions.

@jason-evans-genesys
Copy link
Collaborator

I tested all the components that this PR touched and I also did not see any regressions.

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

Successfully merging this pull request may close these issues.

4 participants