Snippetbar Rework: Popovers, Anchor Positioning, Enhancements #3868
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Making some changes to the reset.less so that some default UA button styling is removed. Then, changing core.less so that the classic "HB" button styling is scoped to a certain class `.colorButton`. This will make it easier to use the button element in other places.
Remove a bunch of lines of code determining "hours ago" elapsed time ranges, replacing it with moment.js functions. Moment is already installed in the repo. Change the `title` strings to include formatted date/time strings and a little more info. Add a "restore from..." header to the dropdown to provide a little context clue for what the buttons do.
This is not related to the overall PR, but it's breaking things if no dismisskeys property is set in localStorage. Adds a default value (empty array).
Clicking a menu item that opens a submenu shouldn't trigger a snippet generator, it should only open the submenu. This commit removes the gen methods from these types of snippets.
Most changes here are to the nodes that contain the snippet buttons and menus, and the snippets themselves. The snippetbar now contains two subgroups: snippets, and the "other ones". The "other ones" are the smaller buttons, called 'tools' and includes the editors (which is a somewhat awkward grouping). The 'tools' are further grouped into history tools, code tools (aka CM tools), and the editor buttons. Snippets get revamped into html `popover`s with CSS anchor positioning. They become click-only activated.
The changes to this file are numerous, and I because I removed a lot of redundant code and re-ordered stuff there are few untouched lines in the diff at this point.
|
Should we block this until firefox comes around then? |
|
Do you mind also disabling the editor themes button in the properties panel? given that this PR touches snippetbar anyway. |
This is not necessary anymore. Related to this PR, in the PR to make the snippetbar wrap correctly, i may have introduced a visual issue we could fix here:
|
|
What are the downsides if we add the polyfill - assuming the polyfill plays nice? |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Background
This PR is primarily an effort to bring forward the use of the HTML attribute
popoverand the new CSS Anchor Positioning API (view in Chrome). Right off the bat I'll say I'm not sure we want to go this route right away, because we are still waiting on Firefox to rollout anchor positioning. I'm eternally optimistic that it will be soon. But hopefully this can at least be illustrative of howpopoverand anchoring can work together in a UI.Using the
popoverattribute in an element means that the element is rendered to a top layer root element, basically a sibling to the<html>element. This means it is higher in the layers than any other item, and so can't be clipped/hidden behind anything except other elements that come after it in top layer. In the DOM, the element is still wherever you put it in your code, and can be styled just as you would normally expect, with a couple of exceptions....it's just rendered above everything else.Popovers do lose some ability to position them. In Firefox, where Anchor Positioning isn't a thing yet, this makes it difficult to place things like the menus in the right spot because your only option is to use absolute positioning in relation to the viewport; you cannot position it relative to a parent/ancestor. In Chrome, you can use Anchor Positioning on popover elements, which pretty much covers all our bases.
Anchor positioning is for positioning an element in relation to another. Notably, the two elements do not need to be related to each other in the DOM-- they don't need to be parent/child/sibling etc. You just give an element an anchor name, and then reference that name in the other element's css anchor position property. To set where the anchored element is positioned in relation to the anchor, there are a few easy keyword-based properties that are basically "position the anchored element's left top corner against the anchor's right top corner". You can do more with sizing, margins, etc. Anchor positioning can dynamically keep an element within the viewport until the element would otherwise be totally out of view, or switch the positioning if the anchored element doesn't fit anymore, etc. Anchored elements can even have different corners anchored to different elements.
So what, ignore Firefox?
So the big downer is that because Firefox doesn't have Anchor Positioning yet, it's actually pretty difficult (as far as i can tell), to have a decent fallback. Popover elements lack some positioning abilities that would help mitigate the issue. As this PR currently stands at time of writing, the Firefox experience is pretty disappointing. Some options:
This is a big PR, what's in here anyways?
This started off as a quick effort to show that Anchor Positioning and Popovers are the way to fix the "menus clipping behind the preview pane" problem. But as I had to make a little adjustment here and there, things got out of hand. Particularly with snippetBar.less, which has been significantly altered.
buttonso that UA styling is reset. Update core.less so that not all buttons have the dramatic styling applied to them which later need to be overridden.Even if this PR is rejected, I think there is a lot of value in taking the changes to metadataEditor.less as their own thing and implementing them in another PR, which is what I should have done before I got carried away.
What's left?
Fix the firefox issue.
And I still think there is opportunity for better display of the snippet bar, particularly around the editor tabs. They should have full text labels since they are very important buttons and have the least intuitive icons (in my opinion). But I think that can be tackled in a greater re-envisioning of the editor pane.
I may also take a crack at revising the snippet menu html structure again to handle better keyboard navigation.
Related Issues or Discussions
QA Instructions, Screenshots, Recordings
Right now, I'm mostly interested in what people think of how it actually works in the deployment. Specifically, look at it in Chrome. Also look in Firefox to see how crappy it is.
I will update the reviewer checklist later.
Reviewer Checklist
Please replace the list below with specific features you want reviewers to look at.
*Reviewers, refer to this list when testing features, or suggest new items *
Copy this list