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

Make the HoverBar more visible #724

Merged

Conversation

maxime-rainville
Copy link
Contributor

@maxime-rainville maxime-rainville commented Aug 22, 2019

I've re factored the HoverBar to remove all the timeout logic. I've added a HoverBar at the start of the Elemental area as well. I made the HoverBar focusable and I added an aria label for accessibility.

Parent issues

@maxime-rainville
Copy link
Contributor Author

Need to fix some IE issue.

@maxime-rainville maxime-rainville force-pushed the pulls/4/ticken-the-insert-here-line branch from 42e2b90 to 953b6d9 Compare August 22, 2019 04:01
@maxime-rainville
Copy link
Contributor Author

IE is happy. Ready for review.

'hover-bar__area',
{ 'hover-bar__area--focus': popoverOpen }
);
const label = i18n._t('ElementAddNewButton.ADD_BLOCK', 'Add block');
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I use the same label as the AddNewButton. We might want to add something more contextual like "Insert block".

@maxime-rainville
Copy link
Contributor Author

Copy link
Contributor

@robbieaverill robbieaverill left a comment

Choose a reason for hiding this comment

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

Nice work! Looks great. I've left a couple code related suggestions, and I have one qualm with the UI - the plus sign isn't aligned with the middle of the bar correctly. Tested on Mac with Chrome.

image

client/src/components/ElementEditor/HoverBar.scss Outdated Show resolved Hide resolved
Copy link
Contributor

@bergice bergice left a comment

Choose a reason for hiding this comment

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

LGTM, but I do think the fade in should be 100ms instead of 200ms. For someone who moves their mouse around quickly, they may never notice the hover bar at all.

client/src/lib/prefixClassNames.js Outdated Show resolved Hide resolved
@maxime-rainville
Copy link
Contributor Author

I've centred the (+) icon.

Co-Authored-By: Robbie Averill <[email protected]>
@maxime-rainville
Copy link
Contributor Author

LGTM, but I do think the fade in should be 100ms instead of 200ms. For someone who moves their mouse around quickly, they may never notice the hover bar at all.

The original version had a much longer delay. I think the point of the delay is to avoid having the hover bar pop up everywhere has you move your mouse across the elemental area.

It's a compromise between making the feature easy to discover and having this bar jumping at you all the time. If we make the delay so small that it's absolutely impossible to miss it, then we'll probably end up having this aggressive blue bar irritatingly popping everywhere.

Copy link
Contributor

@robbieaverill robbieaverill left a comment

Choose a reason for hiding this comment

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

A structural suggestion for your consideration

client/src/components/ElementEditor/HoverBar.scss Outdated Show resolved Hide resolved
@robbieaverill
Copy link
Contributor

If we're being picky, there's probably a pixel or so still needing to be moved in the alignment:

image

@maxime-rainville
Copy link
Contributor Author

I've added one pixel. It seems to make things a little bit better in chrome and a little bit worst in Firefox.

Copy link
Contributor

@robbieaverill robbieaverill left a comment

Choose a reason for hiding this comment

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

I like it, just confirming with Jared

@robbieaverill
Copy link
Contributor

One quirk in IE11, when you tab into it and press enter it opens the new block dialog and flashes briefly (might be a reload of something), then the focus is lost and when you close the dialog (esc key) you continue to tab from the top of the screen again. Might be a separate issue, but worth mentioning.

@robbieaverill robbieaverill merged commit 2424f4b into silverstripe:4 Aug 27, 2019
@maxime-rainville maxime-rainville deleted the pulls/4/ticken-the-insert-here-line branch August 27, 2019 22:54
@maxime-rainville
Copy link
Contributor Author

So when you escape the popover in IE11, you would want the focus to be on the same hover bar?

@robbieaverill
Copy link
Contributor

Yeah correct, this is how Chrome behaves for me

@maxime-rainville
Copy link
Contributor Author

Firefox acts more or less the same as a Chrome.

Interestingly, the popover on the green "add block" button seems to return the focus to the start of the page in all browsers. We might need to add an onEscape handler a to the AddElementPopoverComponent that controls where the focus goes when you hit the escape key.

@robbieaverill
Copy link
Contributor

Ok I'll raise another issue to track this - you should be able to spam enter then esc to toggle repeatedly

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

Successfully merging this pull request may close these issues.

3 participants