-
Notifications
You must be signed in to change notification settings - Fork 88
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
fix(NcActionButton): RTL support #6200
base: master
Are you sure you want to change the base?
fix(NcActionButton): RTL support #6200
Conversation
ca9deaa
to
6d4b55a
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #6200 +/- ##
==========================================
+ Coverage 37.86% 42.62% +4.76%
==========================================
Files 148 154 +6
Lines 5261 4040 -1221
Branches 1577 1017 -560
==========================================
- Hits 1992 1722 -270
+ Misses 3185 2206 -979
- Partials 84 112 +28 ☔ View full report in Codecov by Sentry. |
src/assets/action.scss
Outdated
body[dir=rtl] &.arrow-left-icon, | ||
body[dir=rtl] &.chevron-right-icon { | ||
transform: scaleX(-1); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure about this part. It mirrors all the arrow-like icons in NcAction button, link, text elements.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These two arrow icons are used to indicate a direction within the interface.
One way to resolve this issue is to modify the .vue file that sets the icons’ direction based on the interface mode. Alternatively, we can flip these icons horizontally using CSS, which is the approach I took.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These two arrow icons are used to indicate a direction within the interface.
But these styles are wide. It affects all arrow-like icons inside these components. Not specifically "submenu" or "back" buttons.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with @ShGKme
Also I am not sure this should be handled by the library, because there might be use cases for left and right like the physical direction. It should probably be handled by the app instead.
Maybe we should either provide something like NcIconArrowStart
and NcIconArrowEnd
or a directive to flip if is RTL like <IconArrayLeft v-rtl-icon />
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What if we use NcPopover and NcActionButton to come up with something like NcContextMenu which handles submenus and draws arrows if menu item has children.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What if we use NcPopover and NcActionButton to come up with something like NcContextMenu which handles submenus and draws arrows if menu item has children.
Submenu is already (partly) a feature of NcActions
. NcActionButton
renders that chevron-right-icon
for isMenu
buttons.
The back button is not, but could be solved via #6209
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I removed the arrows modifications. Now the PR only fixes the padding.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree about what's mentioned above regarding the icon,
everything else looks good ✅
6d4b55a
to
1984091
Compare
Signed-off-by: Faisal Alghamdi <[email protected]>
1984091
to
d9a0e94
Compare
Hello
This PR fixes padding in RTL mode for NcActionButton.
☑️ Resolves
🖼️ Screenshots