Follow up review of Betterbuttons ported into core

[sachajudd]

Follow up design review and comments following Better buttons which was ported into core last year.
Reference: #436

Although very close to what we envisioned currently core does not match the initial designs pitched. Seeing what we currently have I've reconsidered the "Previous" and "Next" buttons due to catering for disabled states. See the Styleguide for the new designs and I'm open to feedback if anyone has any suggestions. We haven’t added any buttons without text yet in the CMS but because these are new custom styles I think we can achieve the same result we initially pitched for.

Design and functionality review:

• Plus-circle width should be 26 x 26px
• Something a bit strange going on with the hover and border of the plus-circle
  
• The element currently is not accessible (tested using Axe) and will need to have aria-labels for people who use screen readers and can be used when tabbing throughout the CMS.
• It would be great to add the tooltips as this will help new users who have never used BetterButtons to understand the component before using it.
• As @ScopeyNZ suggested in the referenced pull request there is some specific styling and it probably makes sense to define the Betterbuttons styles in BEM format and also use existing variables.
• The loading indicator should be the default page SilverStripe loading indicator rather than inside the Prev or Next buttons. @ScopeyNZ raised a good point about still being able to enter content and press save or publish if you were on slow internet or on a mobile device.

Pull requests

• API Replace FormActions with anchors to enable panel-based loading in GridField navigation buttons silverstripe-framework#8953
• Better buttons UX and design improvements #873

[robbieaverill]

Bump - this is pretty ridiculous for a new feature to be missing so many basic requirements. Happy to pick this up myself if the OSers are stretched for time.

[chillu]

@bergice @unclecheese Reminder that we're committed to creating accessible UI by default (context: #436).

[robbieaverill]

Plus-circle width should be 26 x 26px

I think the way it has been implemented at the moment, the minimum size it can be is 32px

Something a bit strange going on with the hover and border of the plus-circle

I've set the border colour on hover to be transparent which fixes this bug. Primary buttons usually have a border that is slightly darker than the background, but I don't think it's a noticeable change and removes this quirk anyway.

As @ScopeyNZ suggested in the referenced pull request there is some specific styling and it probably makes sense to define the Betterbuttons styles in BEM format and also use existing variables.

I've switched to use more BEM style than was previously regarding circular-group -> btn-group--circular and circular -> btn--circular.

I've put the measurements we use into variables, but I don't think we can use them outside this stylesheet for now because they're quite specific to these circular buttons.

The loading indicator should be the default page SilverStripe loading indicator rather than inside the Prev or Next buttons

This is actually the default behaviour for FormActions. We'd need to convert these to anchors instead (like "Cancel" when you add a new record) in order to get this to show the loading indicator over the CMS panel. Semantically speaking I think it's fine to do that too, because none of the prev, next, add new actions require a post request.

https://github.com/silverstripe/silverstripe-framework/blob/4.3.3/src/Forms/GridField/GridFieldDetailForm_ItemRequest.php#L299-L309

One thing missing though is that the button loading indicators are not designed for buttons with no labels, which could be quite small, which is why the loading indicators in place already look so strange.

[robbieaverill]

I've fixed everything mentioned except for the button sizing. I think it's OK the way it is. We can open a separate follow up issue referencing this if you disagree @sachajudd when you get back.

[unclecheese]

Thanks for all your work on this, @robbieaverill. This wasn't a great merge. It didn't get the scrutiny it merited due to the pressure to get it into 4.3, which didn't even end up happening. It should have been subject to a final review from @sachajudd before merging, and as the peer reviewer, I'll own that.

I'd like to fast-track this and get it in as a post-RC patch level fix, if @chillu is okay with that. I don't think we can confidently deliver this feature in 4.4 with these known issues, especially if we have what appears to be a comprehensive set of fixes for them sitting in peer review.

Thanks again for jumping in.