Skip to content

First step at fixed label and handle support#441

Merged
argyleink merged 2 commits intoGoogleChromeLabs:masterfrom
Kilian:fixed-labels
Apr 15, 2020
Merged

First step at fixed label and handle support#441
argyleink merged 2 commits intoGoogleChromeLabs:masterfrom
Kilian:fixed-labels

Conversation

@Kilian
Copy link
Copy Markdown
Contributor

@Kilian Kilian commented Mar 30, 2020

This ended up needing changes in quite a bunch more files than anticipated, and might need other things to update as well but I don't quite have the overview for that.

My feeling is that visbug-label would benefit from the same API as visbug-handle/visbug-hover, where label.position is given el and boundingClientRect is calculated in the web component (and isFixed is determined there too).

Now label needs other plugins to deal with boundingClientRect, while handle just calculates it itself.

@argyleink
Copy link
Copy Markdown
Contributor

hosted playground https://kilian-labels.glitch.me

Copy link
Copy Markdown
Contributor

@argyleink argyleink left a comment

Choose a reason for hiding this comment

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

This ended up needing changes in quite a bunch more files than anticipated, and might need other things to update as well but I don't quite have the overview for that.

I'll try to do lots of QA on it. Did touch a lot of files, but we did teach a few components a new trick. You did notice a delta where some components have an updated api and others (label) dont. Agree on the suggestion to have label join the rest 👍

PR overall looks good! Does label.element.css need both position supplied?

Comment thread app/utilities/isFixed.js
Comment thread app/components/selection/label.element.js Outdated
Co-Authored-By: Adam Argyle <argyle@google.com>
@Kilian
Copy link
Copy Markdown
Contributor Author

Kilian commented Mar 31, 2020

Does label.element.css need both position supplied?

I followed the way the other custom properties were defined in that file but I'm happy with using a fallback there too like I did in another file.

@argyleink
Copy link
Copy Markdown
Contributor

hm, what should we do with position sticky elements?

@Kilian
Copy link
Copy Markdown
Contributor Author

Kilian commented Apr 2, 2020

I don't know. Unless we make the labels children of the elements I can't think of a way that doesn't involve adding listeners to everything that's position sticky and getting their position on each scroll.

@argyleink
Copy link
Copy Markdown
Contributor

i dont have any good answers either. this PR is pending squash and merge until I can do a local build and verify across a few test sites. tests and staging look good tho!

@argyleink argyleink merged commit 6644909 into GoogleChromeLabs:master Apr 15, 2020
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.

2 participants