CorruptedArk / open-chaos-chess

Open Chaos Chess is a free and open source version of Chaos Chess, stripped of Google Play Services.
GNU General Public License v3.0
48 stars 7 forks source link

I have fixed the problem of the blocked texts in the setting page #50

Closed EasyVector closed 3 years ago

EasyVector commented 3 years ago

Dear developer!

I am the creator of issue #49, and I believe I have fixed the problem of the blocked texts on the setting page. I would appreciate it if you could kindly revise my code and give me some advice. Thank you so much for your precious time!

Here are screenshots after my modifications:

copy copy
CorruptedArk commented 3 years ago

I'm impressed that you found a way to fix this problem so fast, and I congratulate you for that. After a quick look over it, I see that your solution uses fill_parent which is deprecated and uses text sizes in dp instead of sp. So I can't accept this pull request as it is, but I appreciate your work all the same. The quick thinking you showed is very valuable in a developer, and I hope you keep contributing to projects.

EasyVector commented 3 years ago

Thank you for your comments! The reason why I use dp instead of sp is because the text can not change its size when dp is used. In this circumstance, if the text is set to the largest and sp is used, the text might exceed its border and be blocked. Also I guess ‘autoSizeTextType=uniform’ could be used to avoid this problem. And thank you for your reminder that fill parent is deprecated. :)

CorruptedArk commented 3 years ago

I can understand why you used dp text sizes and it's not bad for a quick fix. However, I do want to allow the text to expand and respect a user's setting. So instead of changing that, my rough idea for a solution involves making the views expand to hold the text

EasyVector commented 3 years ago

Or could you accept this pull request after I make some more modifications? Thank you so much :)

CorruptedArk commented 3 years ago

I'd have to review your changes again, but if they're acceptable and solve the problem without breaking something else, I'll gladly merge them in.

EasyVector commented 3 years ago

I can understand this. I would like to think about this as well and improve it. Thanks again :)

CorruptedArk commented 3 years ago

Of course, this project is always open to community contributions, and I am happy to give developers an opportunity to hone their skills

EasyVector commented 3 years ago

Hi! I have made my latest modifications according to your request. First, I reverted my changes about those texts and used sp instead. Now those texts in the scrollview should be able to change their size responsively. Second, I added a margin of 10dp to those color control boxes, and they should be consistent with the boxes above. Third, I managed to set the gravity of all of these texts to "center_vertical", making those texts be situated in the middle of the boxes vertically. And I also refined this code according to your opinions. Thanks again for your patience and kindness!

Here are some screenshots after my modifications:

copy copy copy
CorruptedArk commented 3 years ago

Good work! I made some additional changes in the same vein as you in order to fix some remaining views at some screen sizes.