Suggestion for fixing issue #5

[Safirah]

I added a button and an editView so we are able to change the players name
Also I've wrapped everything around a scroll view and increased the height of the player's tags so it can be better displayed on smaller screens

[adriantache]

Great work, and I like the way you've done it, but I'm not 100% on this pull request. Main issue is the fact that you're also changing things that are unrelated to the issue you're solving. I recommend splitting your work into two pull requests, one for the names and one for the layout, since that's an issue to be solved as well.

Firstly, regarding the functionality you've added, I like the work you've done, and if you could please submit a pull request just for that part, I will merge it immediately. What I would do differently, however (this is optional, just ideas):

• move the Change name button closer to the text itself
• swap out the button for a clickable ImageView with a pictogram (maybe this, inverted): https://thenounproject.com/search/?q=edit%20text&i=767460)
• swap out the submit button for a similar icon
• also edit the reset() method to reset names, maybe (or just reset the EditText)
• if the EditText is blank, set the text back to Playerx
• figure out some new way of displaying winner on game end

Secondly, regarding the layout, the problem is that your fix, while it makes sense, and I appreciate your clean code, has made the app look like this on my device: https://photos.app.goo.gl/5N5CDoaHNhj3nScR2. Whereas if I make the top logo visible again, and give it a manual size of 100dp, it looks as intended: https://photos.app.goo.gl/FRGdHJrBwDmMkt2v2. It seems kind of plain without that top logo, to me. Thing is, the layout is meant for very high DPI, which Google doesn't really let us program for, so for normal DPI the layout looks off. And I wouldn't make the player cards taller, they start to compete with the scores for attention in the design, and it's space we don't have.

As far as layout goes, I'd actually prefer converting it to a ConstraintLayout (with maybe some ScrollLayout for very small screen), and tweaking logo size, submit button and graphic size, maybe rethinking some of the assets, I'm not sure.

[Safirah]

Hello and thank you for your very helpful feedback!
Yes, I had some doubts about adding the Scroll Screen, the main reason I've added all this stuff related to another issue, it's because on smaller screens the edit text's text is not visible at all! (see screenshoots: first screenshoot me writing 'A' on EditText box, secound screenshoot result), the way I solved this problem was by increasing the layout_height of the player tags, but then the app would "overflow". What should I do in this case?

But, regarding the changes you proposed:

• I'll try to move the button closer, it seems a good idea
• I don't know if icons are as explicit as some plain ol' text, I'd rather leave it as it is for now
• Yes, I'll change the reset as well (completly forgot about that one 😃)

if the EditText is blank, set the text back to Playerx

• Yes, good idea, will also implement that!

figure out some new way of displaying winner on game end

• You mean at the end instead of displaying "Player x won" displaying "Player-name won"? I'll try to find a way, it might not look the same since before we were using an image, this won't be possible anymore.
  I'll make another push request as soon as possible,
  Sorry for my late reply
  
  

[adriantache]

Hey, thanks for your comment :)

The thing is, it's generally a bad idea to solve multiple issues in the same pull request, as things can become problematic, as you can see here. I mean, yeah, your changes might be a problem on smaller layouts, but that's a separate problem, than needs a separate solution, primarily a new layout xml, for xxhdpi screens for examples. You can't fix a problem by just creating a different one on other screens. This is why I'm saying that if you want to fix the layout, do that in a separate pull request, and do it properly, without impacting the current functionality and design of the app.

Regarding pictograms, I prefer them for two reasons: they make sense in any language without taking up a lot of space, and they don't compete for attention with the name text itself from a design perspective. But for this revision, text is fine, although I'd probably shorten it to "rename" and maybe change some part of the aspect, like text size or colour to make it less intrusive during gameplay.

Regarding the winner display, I can provide you with any graphics you need. It might not look as great, but Montserrat is a Google Font, so it is possible to make something similar, biggest problem would be scaling text size with screen size, that might make it look off.

[Safirah]

Any suggestions on solving the problem that the text in the textEdit is too small, the screenshoots I've send are on the layout where I didn't change anything on the layout (except adding the TextViews and EditTexts)?

[Safirah]

@adriantache I've been thinking and I'd rather start solving the problem with the smaller screens, have the pull request accepted for that problem and then make the pull to solve this problem, I think I an make this app more responsive without the need to create any extra layouts. Is that okay with you?

[adriantache]

The approach I've taken to fixing layout is defining separate layouts for separate screens and tweaking them. For example, simplest solution would be to increase the text box size as you have. I've done this for both https://github.com/adriantache/Super-Tic-Tac-Toe and https://github.com/adriantache/Big-Five-Personality-Test, and basically did the following things: defined new layouts for multiple screen sizes (I test on a Pixel XL, a Moto G 3 and a Sony C1905, so xxxhdpi+, xhdpi and hdpi) and tweaked certain elements around those screens. For example, in the super tic tac toe app I've hidden the text under the board for smaller screens and implemented a Toast instead. In the personality test, I've set the graph to "fail gracefully", as it simply doesn't show up on small screens, but there's a bit of text below it that gives you the results. I'm still learning this multiple layouts thing myself, but I think it's preferable to have multiple one-screen layouts than to have a single layout that scrolls on some screens and has too much white space on others.

What I would do on smaller screens is to dynamically hide the top image (give it a size of 0dp and a weight, to make it disappear on smaller screens). Honestly, I think the best approach to fixing the layout would be to convert it to a ConstraintLayout and set the optional items to match_constraint (0dp) so they just disappear. I don't think it makes much sense to have a ScrollView in this kind of app.

Why would you prefer not having multiple layout files?