-
Notifications
You must be signed in to change notification settings - Fork 30
fix: reverse is not doing great in Arabic #364
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
base: main
Are you sure you want to change the base?
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
Reviewer's Guide by SourceryThis pull request addresses two key issues: the incorrect display of reversed Arabic text and the inability to pause the marquee on mobile devices. RTL support was implemented by adding a toggle to change the Sequence Diagram for Mobile Pause/ResumesequenceDiagram
participant User
participant Vue3Marquee Component
User->>Vue3Marquee Component: touchstart (Touch on Marquee)
activate Vue3Marquee Component
Vue3Marquee Component->>Vue3Marquee Component: mouseDown()
Vue3Marquee Component->>Vue3Marquee Component: pause()
Vue3Marquee Component-->>User: Marquee Paused
deactivate Vue3Marquee Component
User->>Vue3Marquee Component: touchend (Release Touch)
activate Vue3Marquee Component
Vue3Marquee Component->>Vue3Marquee Component: mouseUp()
Vue3Marquee Component->>Vue3Marquee Component: resume()
Vue3Marquee Component-->>User: Marquee Resumed
deactivate Vue3Marquee Component
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
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.
Hey @kingmui - I've reviewed your changes - here's some feedback:
Overall Comments:
- The CSS variables for scroll direction are a nice touch, but consider if they might conflict with other CSS in the project.
- The mobile pause/play fix looks good, but it might be better to use a more robust method for detecting mobile devices.
Here's what I looked at during the review
- 🟢 General issues: all looks good
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
🤔 This is a ...
🔗 Related Issues
💡 Background and Solution
1. Reverse is not doing great in Arabic
Typically Arabic web pages require a
dir
attribute on thehtml
tags to indicate the direction of the text, which you were unable to reproduce when you originally tested in issue #295 because thedir
attribute was not specified to set the direction of the text.2. Can't click pause on mobile device
Add
touchstart
andtouchend
events to support the click pause feature on mobile device.Since the
mouseleave
event is unreliable on mobile, themouseenter
event is blocked on mobile device.Before
After
📝 Change Log
Summary by Sourcery
Fix text direction issues in RTL languages like Arabic, and improve pause functionality on mobile devices. Toggle the text direction by adding a
dir
attribute to thehtml
element. Addtouchstart
andtouchend
event listeners to the marquee component to support pausing on click on mobile devices. Remove themouseenter
event listener on mobile devices to avoid blocking the marquee. Add a button to toggle the text direction between left-to-right and right-to-left.Bug Fixes: