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

fix: patch to direction:rtl flipped the content. fixing logic instead #295

Merged
merged 3 commits into from
Jun 18, 2022
Merged

fix: patch to direction:rtl flipped the content. fixing logic instead #295

merged 3 commits into from
Jun 18, 2022

Conversation

eyal-elkevity
Copy link
Contributor

  • Please check if the PR fulfills these requirements
  • The commit message follows our guidelines
  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been added / updated (for bug fixes / features)
  • Unit tests are passing ng test
  • Lint tests are passing ng lint
  • What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
    bug fix

  • What is the current behavior? (You can also link to an open issue here)
    after my last commit to support direction:rtl, Move functions worked but the content got flipped. See I wanted to keep the existing "math" logic and just trick the browser... but the content got flipped. so I removed the flipping code and instead changed the scrollLeft calcs to support negatives (it was just 3 tiny places. rest of commit is actually removing of the old fix).

  • What is the new behavior (if this is a feature change)?

  • Does this PR introduce a breaking change? (What changes might users need to make in their application due to this PR?)
    no

  • Other information:

@eyal-elkevity
Copy link
Contributor Author

I've added another commit to this PR, sorry I thought it would create a new one. it's just that if you don't use scrollbar-hidden="true" - when calling moveTo() with a high number (like in the demo, click the "last" button) - it actually moves the index there, because calculating the containerWidth fails. This happens since neither local members "wrapper" nor "parentNode" are initialized. so not to harm existing code - I didn't force any initialization - just added another "||" to the width calculation with the actual parent.

@eyal-elkevity
Copy link
Contributor Author

ok nm I've reverted the scrollbar patch because a test fails, seems to be that in test mode the moveTo fails, not sure why. it's late for me so I'd rather just revert and have this PR focus on the direction:rtl anyways. sorry...

@bfwg
Copy link
Owner

bfwg commented Jun 17, 2022

Should we revert #293?

@eyal-elkevity
Copy link
Contributor Author

Should we revert #293?

well actually this commit cleans the code from that commit and introduces an actual fix to direction:rtl

Copy link
Owner

@bfwg bfwg left a comment

Choose a reason for hiding this comment

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

LGTM

@bfwg bfwg merged commit 1886c02 into bfwg:develop Jun 18, 2022
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.

2 participants