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:[BUG] Key Labels Not Displaying in Landscape Mode #17022

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 5, 2024

Purpose / Description

Key Labels Not Displaying in Landscape Mode

Fixes

Approach

use linear layout

How Has This Been Tested?

Tested on Android Device

UI Screenshots

WhatsApp Image 2024-09-13 at 11 26 13 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
Member

@BrayanDSO BrayanDSO left a comment

Choose a reason for hiding this comment

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

The issue can be resolved without creating a new layout, which makes the code easier to mantain, so please do that.

Also, this breaks the layout in tablets and screens large enough to properly show the dialog

@BrayanDSO BrayanDSO added the Needs Author Reply Waiting for a reply from the original author label Sep 5, 2024
@Prince-kushwaha
Copy link
Contributor Author

@BrayanDSO incorporated feedback

@Prince-kushwaha
Copy link
Contributor Author

@Arthur-Milchior can u please review this pr

@BrayanDSO
Copy link
Member

Please don't ping maintainers in such a hurried manner to get this reviewed. This isn't a high priority issue and we will get it whenever we can. Respect the time and availability of other people, please.

Copy link
Member

@BrayanDSO BrayanDSO left a comment

Choose a reason for hiding this comment

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

Dialog broken

@Arthur-Milchior
Copy link
Member

Please, ideally use a single commit. You can edit your commit and force push the branch.
Use multiple commit if your work is done in multiple steps, to ease review

@Arthur-Milchior
Copy link
Member

FYI, you'd make the review faster if you could also provide screenshot for vertical, and tablet. So that it's clear it works everywhere.
And, more importantly, if you could explain why the change was required. What was broken and now works

@Arthur-Milchior
Copy link
Member

Dialog broken

Running on the current version of the PR, the dialog seems okay to me. I admit I don't see what's broken.

The dialog is smaller. But I must admit that I don't see any problem with not having it take a lot more space.

@Arthur-Milchior Arthur-Milchior added the Needs Second Approval Has one approval, one more approval to merge label Sep 10, 2024
Copy link
Member

@BrayanDSO BrayanDSO left a comment

Choose a reason for hiding this comment

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

Just to clarify what's broken: the dialog looked a certain way before in portrait mode or in big screens like tablets, and this PR breaks that look while it can easily preserve it and fix the issue at the same time. Don't break something to fix another.

@Prince-kushwaha
Copy link
Contributor Author

@BrayanDSO I tried to preserve the look of the dialog box in portrait mode but it is not possible using signal layout.xml that preserve portrait mode dialog look and also have responsive in landscape mode.

@Prince-kushwaha
Copy link
Contributor Author

if have any solution for that .please share it with me

@Prince-kushwaha
Copy link
Contributor Author

Hi @BrayanDSO , i fixed the dialog broken issue
WhatsApp Image 2024-09-13 at 11 26 13 PM (1)

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.

please do not include merge commits
please squash changes to a single commit with descriptive commit title + description

@mikehardy mikehardy removed the Needs Author Reply Waiting for a reply from the original author label 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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] Key Labels Not Displaying in Landscape Mode
4 participants