Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

fix: export option dialog have text overlapping issue and last checkbox is not visible in landscope mode #17061

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Prince-kushwaha
Copy link
Contributor

@Prince-kushwaha Prince-kushwaha commented Sep 12, 2024

Purpose / Description

export option dialog have text overlapping issue and last checkbox is not visible in landscope mode

Fixes

Approach

make layout scrollview

How Has This Been Tested?

physical android device

Screenshot

WhatsApp Image 2024-09-12 at 8 26 06 PM
WhatsApp Image 2024-09-12 at 8 26 08 PM

Checklist

Please, go through these checks before submitting the PR.

  • You have a descriptive commit message with a short title (first line, max 50 chars).
  • You have commented your code, particularly in hard-to-understand areas
  • You have performed a self-review of your own code
  • UI changes: include screenshots of all affected screens (in particular showing any new or changed strings)
  • UI Changes: You have tested your change using the Google Accessibility Scanner

Copy link
Contributor

@criticalAY criticalAY left a comment

Choose a reason for hiding this comment

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

LGTM, but PR can be rebased

Copy link
Member

@mikehardy mikehardy left a comment

Choose a reason for hiding this comment

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

Agreed with @criticalAY - @Prince-kushwaha we use the merge queue here set to rebase, and I can't rebase-merge a dirty commit history like this. If you'll be making a bunch of PRs (and there are two in the queue now for review) you need to make PRs with a clean commit history

In your local checkout of this branch you want to git rebase and squash these 4 commits to a single commit with descriptive title and description

There should never be commits that are meaningless. like "change"

For me that would be this in a terminal:

  • git rebase -i HEAD~4 (to get the last 4 commits up in a commit editor
  • in editor delete pick from commits 2-4 and switch to s for squash
  • now save the editor and it will make a single commit with all messages together in an editor
  • in the editor, delete all the commit messages and make one clean descriptive commit message
  • save that, then git push --force-with-lease to push that rebase to the branch on github
  • re-request review

Copy link
Contributor

@criticalAY criticalAY left a comment

Choose a reason for hiding this comment

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

Much cleaner commit history

@Prince-kushwaha
Copy link
Contributor Author

@mikehardy Thank you for sharing that with me! I’ve learned a lot from that.

@mikehardy mikehardy added UI Needs Review and removed Needs Author Reply Waiting for a reply from the original author labels Sep 20, 2024
Copy link
Member

@mikehardy mikehardy left a comment

Choose a reason for hiding this comment

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

commit history cleaner, and this seems like an okay fix
I know Brayan has strong opinions on UI so asking for a review there
May take a while though, these are not super high priority fixes, so please have patience

@mikehardy mikehardy added Needs Second Approval Has one approval, one more approval to merge and removed Needs Review labels Sep 20, 2024
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
Needs Second Approval Has one approval, one more approval to merge UI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG]: export option dialog have text overlapping issue and last checkbox is not visible in landscope mode
3 participants