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

Fix vertical offset of dismissal option for hardware device connection view #5734

Closed
markmhendrickson opened this issue Aug 7, 2024 · 7 comments · Fixed by leather-io/mono#379 or #5796

Comments

@markmhendrickson
Copy link
Collaborator

Screen.Recording.2024-08-07.at.12.34.07.mov
@markmhendrickson
Copy link
Collaborator Author

markmhendrickson commented Aug 7, 2024

Also seen in this state:

image

@pete-watters
Copy link
Contributor

I took a look at this as I will update the UI library anyway and it is quick to solve but adding good value. The problem is not having a minimum header height in dialog headers when there is no title.

I just want to double check that the other dialog headers with title have the correct height.

An example of this is Sign out which looks like this:
Screenshot 2024-08-20 at 07 08 20

I can change it to have the same height as the non-title ones or else handle those on a specific case basis. If change them all it will look like this on dialogs with title (slightly taller) :
Screenshot 2024-08-20 at 07 09 08

Here is a video that shows it more clearly:

Area.mp4

Let me know what you think @fabric-8 @markmhendrickson @mica000

@pete-watters
Copy link
Contributor

@fabric-8 confirmed it's OK here: https://trustmachines.slack.com/archives/C05LRS7G44R/p1724170032342449?thread_ts=1724166569.243219&cid=C05LRS7G44R

after checking and from what I can see headers are supposed to have a unit-height of 8.5 (68px) — So yes, I think you could apply that same logic to title-less headers with (ie header with just a close button)

@pete-watters
Copy link
Contributor

I merged a fix to the mono repo UI library. Next I need to update this in the extension

@pete-watters
Copy link
Contributor

Extension integration PR open here

@markmhendrickson
Copy link
Collaborator Author

Should we add a (screenshot-based) integration test for this fix?

@pete-watters
Copy link
Contributor

Should we add a (screenshot-based) integration test for this fix?

We should. I will investigate screenshot based testing in here https://github.com/leather-io/issues/issues/264 and include this

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