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.

Sign up for GitHub

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

Jump to bottom

Overlapping fonts #5096 #5813

Merged
merged 6 commits into from
Mar 31, 2021
Merged

Overlapping fonts #5096 #5813

merged 6 commits into from
Mar 31, 2021

Conversation

wangear
Copy link
Contributor

@wangear wangear commented Mar 12, 2021 •
edited by Stypox
Loading

issue : #5096
changed : sp -> dp

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

To fix bug, I converted some string attributes from sp to dp.

Fixes the following issue(s)

APK testing

bugfix_ui
On the website the APK can be found by going to the "Checks" tab below the title and then on "artifacts" on the right.

Due diligence

@AudricV AudricV added bug Issue is related to a bug GUI Issue is related to the graphical user interface labels Mar 12, 2021
@TobiGr TobiGr mentioned this pull request Mar 12, 2021
Closed
This was linked to issues Mar 12, 2021
List item display overlaps #1292
Closed
Overlapping fonts #5096
Closed
@TobiGr
Copy link
Contributor

TobiGr commented Mar 12, 2021

That does not really look like a good solution to me, because sp is a relative unit which is specifically for texts as it links to the user's preferred font size. Users might have a good reason to use a larger font size. If they cannot read the texts, because they are too small, this PR does not help them.
I'd rather suggest to only show the first line of the stream's title if there is not enough space.

@wangear
Overlapping fonts TeamNewPipe#5096
b5ce2eb 
issue : TeamNewPipe#5096
changed :
- rollback dp->sp.
- If additional textView is overlapped, only title view shows.
@wangear
Copy link
Contributor Author

wangear commented Mar 13, 2021

All right. So I did only show the first line of the stream's title if there is not enough space.

@TobiGr TobiGr force-pushed the dev branch 2 times, most recently from 679bc75 to 2aeccc0 Compare March 16, 2021 08:24
@wangear
Copy link
Contributor Author

wangear commented Mar 20, 2021

Please comment about my pr.

wangear added 2 commits March 23, 2021 11:04
@wangear
Overlapping fonts TeamNewPipe#5096
fb02db7 
issue : TeamNewPipe#5096
changed :
- Remove treeObserve and hiding logic.
- RelativeLayout -> ConstraintLayout.
- layout size fixed -> wrap_content.
- if text size is bigger, layout height bigger too.
@wangear
Overlapping fonts TeamNewPipe#5096
bd25eff 
issue : TeamNewPipe#5096
changed :
- remove unusable variable
@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

Stypox
Stypox previously approved these changes Mar 25, 2021
View reviewed changes
Copy link
Member

@Stypox Stypox left a comment

Choose a reason for hiding this comment

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

Thank you! This fixes the issue, and I also really like the fact that some XML files were converted to ConstraintLayout. This should be done for each one, using RelativeLayout sucks ;-)
This will be merged after 0.21.0 is released.

@wangear wangear closed this Mar 31, 2021
@wangear wangear reopened this Mar 31, 2021
@wangear
Copy link
Contributor Author

wangear commented Mar 31, 2021

oh..my god. 2 issues mixed.

Stypox
Stypox approved these changes Mar 31, 2021
View reviewed changes
Copy link
Member

@Stypox Stypox left a comment

Choose a reason for hiding this comment

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

Looks good and works well. Thank you!
Btw, making a single PR for two issues is not a good idea, it's better to separate them.
Also, next time create your PRs from a branch named according to your change, not dev, otherwise you won't be able to make multiple PRs.
And add a "Fixes " before the issues you fixed so that they are closed automatically when the PR is merged.

@Stypox Stypox merged commit 73cfa54 into TeamNewPipe:dev Mar 31, 2021
@wangear
Copy link
Contributor Author

wangear commented Mar 31, 2021

@Stypox Thank you. And I apologize my mistake. I will take you advice.

This was referenced Apr 11, 2021
Closed
Merged
tossj pushed a commit to tossj/NewPipe-legacy that referenced this pull request Apr 23, 2021
@wangear @tossj
Fix overlapping fonts and crash on tapping anywhere on video after lo…
e29d0c3 
…ng-pressing 'Popup' button (TeamNewPipe/NewPipe#5813)

* Overlapping fonts TeamNewPipe/NewPipe#5096
issue : TeamNewPipe/NewPipe#5096

* Overlapping fonts TeamNewPipe/NewPipe#5096
issue : TeamNewPipe/NewPipe#5096
changed :
- If additional textView is overlapped, only title view shows.

* Overlapping fonts TeamNewPipe/NewPipe#5096
issue : TeamNewPipe/NewPipe#5096
changed :
- Remove treeObserve and hiding logic.
- RelativeLayout -> ConstraintLayout.
- layout size fixed -> wrap_content.
- if text size is bigger, layout height bigger too.

* Overlapping fonts TeamNewPipe/NewPipe#5096
issue : TeamNewPipe/NewPipe#5096
changed :
- remove unusable variable

* Crash on tapping anywhere on video after long-pressing 'Popup' button TeamNewPipe/NewPipe#5804
issue : TeamNewPipe/NewPipe#5804
changed :
- checked null
- fixed NullPointerException.
@ktprograms ktprograms mentioned this pull request Aug 25, 2021
Merged
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Stypox Stypox Stypox approved these changes

Assignees
No one assigned
Labels
bug Issue is related to a bug GUI Issue is related to the graphical user interface
Projects
None yet
Milestone
No milestone
4 participants
@wangear @TobiGr @Stypox @AudricV