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

No enough padding on Continue button style #4504

Closed
Chandra-Sekhar-Bala opened this issue Aug 16, 2022 · 12 comments · Fixed by #4553
Closed

No enough padding on Continue button style #4504

Chandra-Sekhar-Bala opened this issue Aug 16, 2022 · 12 comments · Fixed by #4553
Assignees
Labels
Z-ibt Temporary label for Ben to keep track of issues he's triaged.

Comments

@Chandra-Sekhar-Bala
Copy link
Contributor

Describe the bug
Continue has not enough padding when reading size changed to Extra Large

To Reproduce
Steps to reproduce the behavior:

  1. Go to 'Navigation menu' -> Select Option
  2. Click on 'Reading text size' -> Select Extra Large
  3. Open any lesson
  4. Scroll down to click on Continue

Expected behavior
Continue button should have good padding from both ends.

Demonstration
If applicable, add screenshots or videos to help explain your problem.
oppia_issue

Environment

  • Device/emulator being used: Pixel 4 XL
  • Android or SDK version: API Level 27
  • App version: 1.0
@BenHenning
Copy link
Member

@Chandra-Sekhar-Bala have you checked whether this issue repros on the latest develop? Given the recent work on this exact issue, I'd expect this to not be an issue anymore.

@Chandra-Sekhar-Bala
Copy link
Contributor Author

Hey @BenHenning Yes, I have checked it is on the latest version. Steps are mentioned to reproduce the bug, you can verify.

@BenHenning
Copy link
Member

@Ryggs can you look into this since #4385 was supposed to fix this issue?

@Ryggs
Copy link
Contributor

Ryggs commented Aug 22, 2022

Hey @BenHenning taking a look

@Chandra-Sekhar-Bala
Copy link
Contributor Author

@BenHenning @Ryggs let me give my view on this,
Yes #4385 solved the alignment issue. But if you look into the style named @style/StateButtonActive you'll see padding attribute is missing. And that is the reason of this bug.
I hope it helps :)

@Ryggs
Copy link
Contributor

Ryggs commented Aug 22, 2022

@Chandra-Sekhar-Bala The padding attribute is global as we are using a wrap_content and got rid of setting a max_width, hence the button behavior

@Ryggs
Copy link
Contributor

Ryggs commented Aug 22, 2022

Hi @BenHenning Just checked this out and this was the fix accepted by C/O @rt4914 . What would be the workaround here? The text fits overlay when in text size LARGE on phone.
Here's the tablet view
image

@Chandra-Sekhar-Bala
Copy link
Contributor Author

@Chandra-Sekhar-Bala The padding attribute is global as we are using a wrap_content and got rid of setting a max_width, hence the button behavior

So do you prefer adding padding attribute to the style?

@Chandra-Sekhar-Bala
Copy link
Contributor Author

I've added padding attribute which tends to solve this one I guess
Here's an example, when padding is enabled with text size as Medium and Extra large.

Medium:

M

Extra Large:

XL

Looking forward to know your opinion about this @Ryggs @BenHenning !

@BenHenning
Copy link
Member

I haven't been as involved with the nature of this particular issue or the different solutions considered. @rt4914 since you have more context, could you share your thoughts on the ongoing discussion here?

@rt4914
Copy link
Contributor

rt4914 commented Aug 31, 2022

@Chandra-Sekhar-Bala Yes adding padding is a better way to solve this. Basically follow these guidelines:

  • Avoid giving fixed width/height
  • Add padding to all sides of text -- inside button
  • Use sp for textsize and dp for everything else.

These guidelines should be sufficient to solve this issue.

@Chandra-Sekhar-Bala
Copy link
Contributor Author

@rt4914 Got it, thanks. I will make a PR :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Z-ibt Temporary label for Ben to keep track of issues he's triaged.
Development

Successfully merging a pull request may close this issue.

4 participants