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

Use horizontal layout for DateTime picker prompt #1018

Merged
merged 1 commit into from
Sep 29, 2023
Merged

Use horizontal layout for DateTime picker prompt #1018

merged 1 commit into from
Sep 29, 2023

Conversation

HollowMan6
Copy link
Collaborator

@HollowMan6 HollowMan6 commented Sep 26, 2023

 —————————————
|      |      |
| DATE | TIME |
|      |      |
———————————————
|   BUTTONS   |
 —————————————

So that we don't need to scroll

Resolve #998

com igalia wolvic-20230926-235831

@svillar
Copy link
Member

svillar commented Sep 27, 2023

I find this sensible. @felipeerias what do you think?

I just have doubts about hardcoding sizes of UI elements. In general I think we should use proportions or values that we can refer to some others, but not sure how feasible is that in Android UIs in general.

@HollowMan6
Copy link
Collaborator Author

I just have doubts about hardcoding sizes of UI elements. In general I think we should use proportions or values that we can refer to some others, but not sure how feasible is that in Android UIs in general.

Yeah, I agree, but since we are hardcoding basically all the sizes of UI elements, it won't be meaningful if we just change for this PR specifically:

<!-- Prompts -->
<dimen name="prompt_width">475dp</dimen>
<dimen name="prompt_height">525dp</dimen>
<dimen name="prompt_content_max_height">450dp</dimen>
<dimen name="prompt_choice_min_height">300dp</dimen>
<dimen name="prompt_choice_max_height">520dp</dimen>
<dimen name="prompt_min_height">400dp</dimen>
<dimen name="prompt_file_width">600dp</dimen>
<dimen name="prompt_file_height">600dp</dimen>
<dimen name="prompt_datetime_width">1000dp</dimen>

 —————————————
|      |      |
| DATE | TIME |
|      |      |
———————————————
|   BUTTONS   |
 —————————————

So that we don't need to scroll

Resolve #998

Signed-off-by: Songlin Jiang <sjiang@igalia.com>
@javifernandez
Copy link
Member

I just have doubts about hardcoding sizes of UI elements. In general I think we should use proportions or values that we can refer to some others, but not sure how feasible is that in Android UIs in general.

Yeah, I agree, but since we are hardcoding basically all the sizes of UI elements, it won't be meaningful if we just change for this PR specifically:

<!-- Prompts -->
<dimen name="prompt_width">475dp</dimen>
<dimen name="prompt_height">525dp</dimen>
<dimen name="prompt_content_max_height">450dp</dimen>
<dimen name="prompt_choice_min_height">300dp</dimen>
<dimen name="prompt_choice_max_height">520dp</dimen>
<dimen name="prompt_min_height">400dp</dimen>
<dimen name="prompt_file_width">600dp</dimen>
<dimen name="prompt_file_height">600dp</dimen>
<dimen name="prompt_datetime_width">1000dp</dimen>

Kind of agree with @HollowMan6 and it's consistent with what we have. We could implement a refactoring to get rid of all the UI hardcoded values in a follow up patch.

Copy link
Member

@svillar svillar left a comment

Choose a reason for hiding this comment

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

OK let's merge it then. I agree it looks much better without the scroll. Maybe for a future PR we could make the widgets smaller, they're too large for my taste, I don't think we need them to be that size, nor even with hand tracking I think. Anyway I'm deviating from the topic now :)

@svillar svillar merged commit 957cd57 into main Sep 29, 2023
18 checks passed
@svillar svillar deleted the datetime branch September 29, 2023 14:53
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.

Date picker can be too tall
3 participants