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

Add how to remove tab from main page text #6965

Merged
merged 4 commits into from
Aug 31, 2021

Conversation

ktprograms
Copy link
Contributor

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

  • Added a message on how to remove an item from the content of main page.

Before/After Screenshots/Screen Record

Before

Before

After

After

Fixes the following issue(s)

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.

Due diligence

@Stypox
Copy link
Member

Stypox commented Aug 24, 2021

That text in that place looks odd. I would add a subtitle to the toolbar (this should be easily doable without adding new views, but just with .setSubtitle();) saying "Swipe items to remove them"

@ktprograms
Copy link
Contributor Author

I've moved it to the subtitle, but it seems if the text is too long it just cuts it instead of wrapping?

Text Cropped

@Stypox
Copy link
Member

Stypox commented Aug 24, 2021

@ktprograms what about "Swipe to remove"?

@ktprograms
Copy link
Contributor Author

@ktprograms what about "Swipe to remove"?

@Stypox The cropping is only really a problem if the text is too long in different translations.

app/build.gradle Outdated Show resolved Hide resolved
@triallax triallax added the GUI Issue is related to the graphical user interface label Aug 24, 2021
@Stypox
Copy link
Member

Stypox commented Aug 24, 2021

The cropping is only really a problem if the text is too long in different translations.

Mmmh, yes... I don't know what we should do. The fragment title would also be cropped in some locales, but we never cared about that as that's just the default android behaviour.

Maybe you could add a centered description text at the top of the activity with appropriate padding above and below, similarly to the "Edit each notification..." text at the top of the action button section in "Notification" settings.

@ktprograms
Copy link
Contributor Author

The fragment title would also be cropped in some locales

Is there any plan to solve this in future?

@Stypox
Copy link
Member

Stypox commented Aug 25, 2021

Is there any plan to solve this in future?

I don't think so, that's just the way Android works... But titles are usually small (i.e. single words) even when translated.

@ktprograms
Copy link
Contributor Author

I don't think so, that's just the way Android works... But titles are usually small (i.e. single words) even when translated.

Ok, in that case I think I'll just follow your suggestion to have the text at the top and centered.

@Stypox
Copy link
Member

Stypox commented Aug 25, 2021

Ok, in that case I think I'll just follow your suggestion to have the text at the top and centered.

Sorry, I don't get how this is related to "I don't [...] translated.". I was talking generally about the title in the toolbar that you can see in almost every activity and fragment throughout the app.

@ktprograms
Copy link
Contributor Author

I meant that the current approach works for titles since they're quite short, but for this help text I'll put it below the ActionBar.

@Stypox
Copy link
Member

Stypox commented Aug 25, 2021

Yes, and make sure the text can wrap

@ktprograms
Copy link
Contributor Author

ktprograms commented Aug 25, 2021

@Stypox why does fragment_choose_tabs.xml use RelativeLayout? If there's no problem, I'd prefer to convert it to ConstraintLayout which I'm more familiar with.

Edit: I noticed this comment you made: #5813 (review). Does your opinion on RelativeLayout vs ConstraintLayout still hold?

@Stypox
Copy link
Member

Stypox commented Aug 26, 2021

If there's no problem, I'd prefer to convert it to ConstraintLayout which I'm more familiar with.

Yes please, I still hate RelativeLayouts, don't worry :-)

@ktprograms
Copy link
Contributor Author

@Stypox done in e95637f

@opusforlife2
Copy link
Collaborator

@ktprograms New screenshot, please?

@ktprograms
Copy link
Contributor Author

New Screenshot

@ktprograms ktprograms requested a review from Stypox August 28, 2021 00:24
@opusforlife2
Copy link
Collaborator

Thanks! That looks great. I was thinking about the centre of the screen, but that would cause the text to be hidden if the user adds more tabs. This position is much better.

Copy link
Member

@TobiGr TobiGr 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!

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.

Perfect, thank you! :-)

@Stypox Stypox merged commit a4c9732 into TeamNewPipe:dev Aug 31, 2021
This was referenced Sep 5, 2021
@ktprograms ktprograms deleted the indication-content-main-page branch October 10, 2021 11:45
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

add indication as to how "content of main page" is edited
5 participants