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

feat: add labels to preview when necessary #147

Merged
merged 7 commits into from
Nov 5, 2024

Conversation

zugdev
Copy link
Collaborator

@zugdev zugdev commented Oct 30, 2024

Resolves #142

labels-preview.mp4

@zugdev zugdev requested a review from 0x4007 as a code owner October 30, 2024 05:19
@ubiquity-os-deployer
Copy link

ubiquity-os-deployer bot commented Oct 30, 2024

@0x4007
Copy link
Member

0x4007 commented Oct 30, 2024

Mixed feelings on how it looks on mobile but the bottom can't be scrolled to all the way at least in iOS now with the labels offsetting the inner height.

@0x4007
Copy link
Member

0x4007 commented Oct 30, 2024

I think the labels would look better on the bottom on mobile for sure.

@zugdev
Copy link
Collaborator Author

zugdev commented Oct 30, 2024

but the bottom can't be scrolled to all the way at least in iOS now with the labels offsetting the inner height.

Do you mean you cant scroll to the bottom of the issue list? I've added another PR to add some padding

@zugdev
Copy link
Collaborator Author

zugdev commented Oct 30, 2024

I think the labels would look better on the bottom on mobile for sure.

Will try it!

@0x4007
Copy link
Member

0x4007 commented Oct 30, 2024

but the bottom can't be scrolled to all the way at least in iOS now with the labels offsetting the inner height.

Do you mean you cant scroll to the bottom of the issue list? I've added another PR to add some padding

It should be in this pull because this pull introduced the problem.

@0x4007
Copy link
Member

0x4007 commented Oct 30, 2024

trim.AAAF3215-AEE1-4DAA-B0FE-2D15EB737713.MOV

@0x4007
Copy link
Member

0x4007 commented Oct 30, 2024

I just realized that the problem is in production. I think another recent pull introduced the problem.

The footer bar is too tall now. I must have overlooked it in another pull.

@zugdev
Copy link
Collaborator Author

zugdev commented Oct 30, 2024

I just realized that the problem is in production. I think another recent pull introduced the problem.

The footer bar is too tall now. I must have overlooked it in another pull.

Yeah me too, it was introduced in #137 when toolbar was refactored. #145 is a quick solution.

@zugdev
Copy link
Collaborator Author

zugdev commented Oct 31, 2024

I think the labels would look better on the bottom on mobile for sure.

Do you have stacked or side by side in mind? I'll attach some quick illustrations for comparison:

Stacked:

image

image

Side by side:

image

image

Side by side can be good if I can reduce buttons a little, but labels will wrap when too big.

IPhone XR demo:

image

@0x4007
Copy link
Member

0x4007 commented Oct 31, 2024

Side by side but if I had to choose stacked I would make the control buttons on top.

@zugdev
Copy link
Collaborator Author

zugdev commented Nov 4, 2024

@0x4007 lmk what you think about this.

Copy link
Member

@0x4007 0x4007 left a comment

Choose a reason for hiding this comment

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

Super clean on mobile. Not sure on desktop but I'm assuming you also did well there.

A bit concerned about those void statements. I'm not sure if an error is thrown if it will be caught.

@0x4007 0x4007 merged commit bd41261 into ubiquity:development Nov 5, 2024
1 of 2 checks passed
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.

Add labels at issue preview body
2 participants