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

PLAT-109159: Fix to position the NavigationButtons properly in FixedPopupPanels in RTL #438

Merged
merged 28 commits into from
Jun 12, 2020

Conversation

ybsung
Copy link
Contributor

@ybsung ybsung commented Jun 11, 2020

Checklist

  • I have read and understand the contribution guide
  • A CHANGELOG entry is included
  • At least one test case is included for this feature or bug fix
  • Documentation was added or is not needed
  • This is an API breaking change

Issue Resolved / Feature Added

In RTL

  • FixedPopupPanels's panel position is shifted to the right.
  • The Navigation Button Icons are flipped.

Resolution

FixedPopupPanels's panel position is shifted to the right.

: Position the panel properly in RTL like the one in LTR

The Navigation Button Icons are flipped.

: Pass the flip prop to the Icon so that the Icon shows properly

Additional Considerations

N/A

Links

PLAT-109159
Related PR: enactjs/enact#2787

Comments

This PR also fixes the PLAT-108977 to flip the navigation button incorrectly in WizardPanels in RTL.

Enact-DCO-1.0-Signed-off-by: YB Sung ([email protected])

ybsung added 2 commits June 11, 2020 10:04
Enact-DCO-1.0-Signed-off-by: YB Sung ([email protected])
Enact-DCO-1.0-Signed-off-by: YB Sung ([email protected])
MikyungKim
MikyungKim previously approved these changes Jun 11, 2020
Copy link
Contributor

@MikyungKim MikyungKim left a comment

Choose a reason for hiding this comment

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

LGTM

@Djspaceg
Copy link
Member

While this does fix the problem, I want to be sure we're fixing it in the best way for a native speaker of RTL languages. are the next and previous button positions expected or backwards, from their perspective.

Honestly, if it were me, I'd recommend that literally everything reverse directions so RTL language speakers get the same experience as LTR language speakers. It's important to be mindful of how people, with perspectives different from our own, perceive the world and their environment.

@MikyungKim
Copy link
Contributor

@ybsung, did you confirm about the prev/next button positions by UX?
Please confirm it.

@MikyungKim MikyungKim dismissed their stale review June 11, 2020 01:34

need to check ux

@ybsung ybsung changed the title PLAT-109159: Fix to keep FixedPopupPanels's current location in LTR and RTL PLAT-109159: Fix to position the NavigationButtons properly in FixedPopupPanels in RTL Jun 11, 2020
Enact-DCO-1.0-Signed-off-by: YB Sung ([email protected])
ybsung added 5 commits June 11, 2020 14:05
Enact-DCO-1.0-Signed-off-by: YB Sung ([email protected])
Enact-DCO-1.0-Signed-off-by: YB Sung ([email protected])
Enact-DCO-1.0-Signed-off-by: YB Sung ([email protected])
Enact-DCO-1.0-Signed-off-by: YB Sung ([email protected])
This reverts commit 35c05fd.
ybsung added 5 commits June 11, 2020 18:17
Enact-DCO-1.0-Signed-off-by: YB Sung ([email protected])
Enact-DCO-1.0-Signed-off-by: YB Sung ([email protected])
Enact-DCO-1.0-Signed-off-by: YB Sung ([email protected])
Enact-DCO-1.0-Signed-off-by: YB Sung ([email protected])
Copy link
Member

@SkylerBaek SkylerBaek left a comment

Choose a reason for hiding this comment

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

LGTM

}

.navCellAfter .navButton {
right: 0;
transform: translate(100%, -50%);

:global(.enact-locale-right-to-left) & {
transform: translate(0, -50%);
Copy link
Member

Choose a reason for hiding this comment

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

This is unexpected to me. I'm curious if there are some other margins at play here that are causing this to not need the same positioning as the LTR version.

Copy link
Contributor Author

@ybsung ybsung Jun 11, 2020

Choose a reason for hiding this comment

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

Hello @Djspaceg

I'm not sure that I understand what you mean, but...

I'm curious if there are some other margins at play here that are causing this to not need the same positioning as the LTR version.

In a chrome browser and RTL, a DOM element is positioned on the right side based on 0 position with left: 0 css styles. So I think we don't need any margin for before navigation button.
The after navigation button also don't need any margin because it is in RTL and the button positions from the right side to the left side.

So I don't think there is any other margin for the buttons. They just positions in the way that a browser supports.

Anyway, I tried to reduce more code without any transformX in RTL and LTR and applied it in c08e524 commit. Could you check it again and please let me know if it is better or not?

Copy link
Member

@Djspaceg Djspaceg Jun 12, 2020

Choose a reason for hiding this comment

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

Sorry, I was mi-remembering the approach we took to position the buttons.

That's an odd approach, and not what I was expecting. LTR is ordered as RTL and RTL is ordered as LTR?

What if we assigned the order using the order rule, or didn't manually adjust the positions at all, but instead just used the flip prop you made available recently. In LTR everything would be the same, and in RTL, the buttons would be on opposite sides and the arrows would still point away from the panel.

Ryan Duffy added 8 commits June 12, 2020 12:14
Signed-off-by: Ryan Duffy <[email protected]>
Signed-off-by: Ryan Duffy <[email protected]>
Signed-off-by: Ryan Duffy <[email protected]>
Signed-off-by: Ryan Duffy <[email protected]>
Signed-off-by: Ryan Duffy <[email protected]>
Signed-off-by: Ryan Duffy <[email protected]>
@ryanjduffy
Copy link
Contributor

Few changes I made:

  • Added Icon prop flip value "auto" for auto-flipped for RTL
  • Adopted iconFlip for navigation buttons and header back button
  • Added SSTs for flip and Button's iconFlip
  • Documented the LESS changes a bit
  • Added a third panel to the sample so we can verify we're going to the next and previous panel (since wrapping would hide that with only two panels).

@ryanjduffy
Copy link
Contributor

Noting that Travis is failing until enactjs/enact#2789 merges

@ryanjduffy ryanjduffy merged commit 83de39c into develop Jun 12, 2020
@ryanjduffy ryanjduffy deleted the feature/PLAT-109159-yb branch June 12, 2020 22:02
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.

5 participants