Skip to content
This repository has been archived by the owner on Jan 11, 2023. It is now read-only.

Improve UI implementation of EventListeners list #7879

Closed
fvsch opened this issue Feb 5, 2019 · 7 comments
Closed

Improve UI implementation of EventListeners list #7879

fvsch opened this issue Feb 5, 2019 · 7 comments

Comments

@fvsch
Copy link
Contributor

fvsch commented Feb 5, 2019

The UI spec: https://mozilla.invisionapp.com/share/H3PYO9EMC9D#/screens/340771972_Event_Breakpoints_1

A few issues:

  • we have strange alignment of icons, checkboxes, etc.
  • we don't respect the metrics outlined in the spec (horizontal rhythm, 20px-tall rows)
  • the arrow icon should be a button so that it can be focusable; we probably need to come up with an accessible label for those buttons too
  • finally, the arrow icon is currently inside the checkbox's label, so when we click the arrow it triggers the checkbox (which we prevent in JS but it still shows some visual feedback in the UI); the button should be outside the label

I'll probably take this bug. :)

@claim claim bot added the not-available label Feb 5, 2019
@harshlele
Copy link
Contributor

hey, can i take this bug?

@fvsch
Copy link
Contributor Author

fvsch commented Feb 6, 2019

Works for me. :)

Some things to note:

@harshlele
Copy link
Contributor

/claim

@claim
Copy link

claim bot commented Feb 6, 2019

Thanks for claiming the issue! 👋

Here are some links for getting setup, contributing, and developing. We're always happy to answer questions in slack! If you become busy, feel free to /unclaim it.

🦊 Debugger team!

@harshlele
Copy link
Contributor

Hey, @fvsch , about this:

Checkboxes use their platform-specific styles. This means we can't always follow the UI spec in a pixel-perfect way because checkboxes could be 12px, 14px or 16px wide.

I think the row height not being 20px is because of the inconsistent checkbox size on different platforms. Couldn't we just set a fixed size for the checkbox on all platforms?

@fvsch
Copy link
Contributor Author

fvsch commented Feb 11, 2019

Couldn't we just set a fixed size for the checkbox on all platforms?

Nope. Changing the checkbox size doesn't work on all platforms. It seems to work alright on macOS, not sure on Windows, and doesn't work on Linux.

Making rows 20px should be possible nonetheless. For example, using flexbox, you could have a display:flex; align-items:center; container for each row, then the different elements as flex items, and the main size could come from the text label with styles like:

padding: 3px 0;
line-height: 14px;

@harshlele
Copy link
Contributor

Hmm ok i made that change

@fvsch fvsch closed this as completed Mar 26, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

3 participants