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

Adjust the playlist bookmark item layout for RTL languages #11024

Open
wants to merge 4 commits into
base: dev
Choose a base branch
from

Conversation

AbdeltwabMF
Copy link

What is it?

  • Bugfix (user facing)
  • Feature (user facing)
  • Codebase improvement (dev facing)
  • Meta improvement to the project (dev facing)

Description of the changes in your PR

The playlists' bookmark items do not align correctly and have a layout overlap issue when the channel name is in a right-to-left (RTL) language, such as Arabic. Therefore, this change focuses on adding a right boundary to the uploader element, similar to the playlist title, to resolve this issue.

Also, make small adjustments to some items to align them with the uploader element.
Screenshot 2024-05-01 021233

Before/After Screenshots/Screen Record

  • Before:
    Screenshot_20240501_020609_NewPipe

  • After:
    Screenshot_20240501_014114

APK testing

The APK can be found by going to the "Checks" tab below the title. On the left pane, click on "CI", scroll down to "artifacts" and click "app" to download the zip file which contains the debug APK of this PR. You can find more info and a video demonstration on this wiki page.

Due diligence

@github-actions github-actions bot added the size/medium PRs with less than 250 changed lines label Apr 30, 2024
@TobiGr TobiGr added the GUI Issue is related to the graphical user interface label May 1, 2024
@github-actions github-actions bot added size/small PRs with less than 50 changed lines and removed size/medium PRs with less than 250 changed lines labels May 1, 2024
@AbdeltwabMF
Copy link
Author

@TobiGr, for some reason, Android Studio cannot symbolically link this file during the local build

However, it magically works when I try to copy the content it points to. I'm unsure why it works here with this symlink and doesn't work on my local build!

@TobiGr
Copy link
Member

TobiGr commented May 8, 2024

Are you using Windows?

@AbdeltwabMF
Copy link
Author

Are you using Windows?

Yes. Windows 11 Pro 23H2

Copy link
Contributor

@snaik20 snaik20 left a comment

Choose a reason for hiding this comment

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

Thanks for making the changes, looking good!

I'm curious about the adjustment to the height. Is there a specific reason it needs to be changed? Could you elaborate on why 51dp was chosen?

If you want to position an element relative to another element, you could consider using constraints like layout_alignParentTop, layout_below similar attributes in RelativeLayout.

Additionally, to help visualize the impact of the changes, would it be possible to share some before/after screenshots with layout bounds enabled or using a layout inspector tool? Seeing the layouts with large font size configurations would also be helpful for review.

@AbdeltwabMF
Copy link
Author

@snaik20, thank you. I am glad to contribute.

Regarding the adjustment to the height, I scaled down the ImageView to match the width of the title and uploader text, as illustrated in the screenshots below, hence the "51dp".

As you suggested, instead of directly setting the static height, I used the layout_alignTop and layout_alignBottom constraints.

Before

image

After

image

After last fix (using layout_alignTop/Bottom)

image

@AbdeltwabMF AbdeltwabMF requested a review from snaik20 May 18, 2024 13:59
Copy link

sonarcloud bot commented May 18, 2024

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
No data about Duplication

See analysis details on SonarCloud

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
GUI Issue is related to the graphical user interface size/small PRs with less than 50 changed lines
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants