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

Fixed the blocked texts on the third "setting up" page #427

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

EasyVector
Copy link

Dear developer:

Hello! I am the creator of issue #426, and I have fixed the blocked texts on the third "setting up" page. I would appreciate it if you could kindly revise my code and leave me some advice. Thank you so much for your precious time!

Here is the screenshot after my changes:

copy

Thanks again! Have a nice day! :

@NoahAndrews
Copy link

NoahAndrews commented Aug 26, 2021

This seems like something that should be fixed in the original XML, not patched over later in Java.

Note that I am not the original developer of OpenBoard.

@EasyVector
Copy link
Author

@NoahAndrews Hello! I have looked through this project and did not find any pertinent XML files, and I think it is this java file that controls the layout of this page. After I applied some changes to this java file, this page seems to be better now as I predicted.

@lightningtyb
Copy link

I strongly agree with your repair method, and I think this issue needs to modify the Java code @yuhuitech 👍

@NoahAndrews
Copy link

NoahAndrews commented Aug 27, 2021

setup_step_action_label is defined in XML here: https://github.com/dslul/openboard/blob/57cac536bb308cfe1e9308f75b52644b62429f8c/app/src/main/res/layout/setup_step.xml#L34-L37

and can be styled in XML here: https://github.com/dslul/openboard/blob/57cac536bb308cfe1e9308f75b52644b62429f8c/app/src/main/res/values/setup-styles.xml#L21-L24

It really only makes sense to change styling in Java when the styling needs to be determined at runtime.

@NoahAndrews
Copy link

By the way, I'm sorry if my comments came off as abrupt. Thank you for contributing!

@EasyVector
Copy link
Author

Not at all! I would like to check that later on my PC, and I also agree with you it would be much more suitable to change XML files instead of java files, because changing java files is more likely to affect layouts of other pages. Thank you so much for reminding me. I will continue working on this. :)

@EasyVector
Copy link
Author

@NoahAndrews Thank you so much! I have already revoked my changes on that java file and applied some modifications to the relevant XML file. So far it works well on my Pixel 2. Here is the screenshot after my changes:

Snipaste_2021-08-27_11-07-23

@NoahAndrews
Copy link

Nice! Though it looks like the intention is for all style attributes to go in the style XML files.

@EasyVector
Copy link
Author

Yeah, I guess maybe the best practice is to let those XML files manage more layout attributes as much as possible, which are related to size, fonts, and so on. And it would be easier for maintenance in such a case. I am still thinking about a better solution for those problems.

@MajeurAndroid
Copy link
Member

@yuhuitech can you try instead to define a minHeight attribute set to setup_step_action_height replacing the layout_height override ? This will keep the layout consistent in the oppposite case of really small text size.
https://github.com/dslul/openboard/blob/57cac536bb308cfe1e9308f75b52644b62429f8c/app/src/main/res/values/setup-styles-common.xml#L61

@MajeurAndroid
Copy link
Member

I think #485 relates to this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants