canonical-web-and-design / build.snapcraft.io

The Snapcraft Build site.
GNU Affero General Public License v3.0
21 stars 26 forks source link

Investigate migrating buttons to vanilla #865

Closed bartaz closed 7 years ago

carlaberkers commented 7 years ago

@bartaz and @richmccartney, could you add notes of why you decided todo breadcrumbs instead of buttons? what problems do we need to solve? what questions do we need to answer?

Thanks! C

bartaz commented 7 years ago

Issues found when converting buttons to Vanilla:

1. Our current button size is a bit different from Vanilla

Vanilla buttons seem to be a bit bigger (more padding) than current BSI buttons. It's not a huge issue, but a bit visible - especially if we only convert some of the buttons, not all of them.

2. Vanilla buttons have transition on hover

Again, small visual difference. Not a huge deal, but if we migrate only some buttons to vanilla only those will have a hover transition and others will not.

3. BSI buttons have bigger/smaller options

We have buttons in different sizes from Vanilla. We have bigger buttons on landing page:

screen shot 2017-07-06 at 13 02 08

and a bit smaller buttons in "Any missing" dialog:

screen shot 2017-07-06 at 13 02 35

We may try to propose bigger/smaller variants to Vanilla or reimplement it first as our custom vanilla theme.

4. BSI has icon in button on landing page

Landing page (bigger) buttons have a GitHub icon in it. Vanilla doesn't provide any specific styling for icons in the buttons, so we implemented it in BSI buttons.

To use Vanilla buttons we may take icon styles out of button component and render icons inline with buttons text (hopefully without needing to change Vanilla button styles).

Currently it's not a huge issue as these buttons are bigger (so not vanilla compatible) anyway.

5. BSI buttons have spinner inside, that requires additional styles

Some BSI buttons (that trigger longer actions, such as adding repos or registering name) may have a spinner in it (replacing the text without changing button size).

screen shot 2017-07-06 at 13 24 44

This requires additional styles on a button to hide button text and position spinner in it. This would either need to be moved to Vanilla as a pattern or implemented ourselves in custom button.

6. Vanilla base button hover color is not visible in dropdowns

BSI 'base' button style is using different (darker) hover colour (#cdcdcd), because our repos table dropdowns use #f7f7f7 for background. Vanilla base button hover color is #f7f7f7 which makes hover totally invisible in dropdowns.

screen shot 2017-07-06 at 13 13 23

We would need to use different button style (or propose new button style to Vanilla to support different background), or create custom button style (again, our own theme), or change dropdown background.

6. Vanilla has additional 'is-inline' button modifier

It's rather an implementation detail than an issue, but Vanilla has special is-inline modifier class for inline buttons (to give them some margin). We don't have such styles in BSI, but it's probably just a matter of adding support for it and starting to use it when appropriate.

7. CSS size increase (a lot of duplication in Vanilla CSS)

Because of the way Vanilla styles are built (from SASS mixins) there is a lot of duplication in button styles (every button variant duplicates all shared button styles). This makes the CSS significantly larger.

Just adding the buttons increased total size of BSI CSS from ~30kB to over ~40kB:

// BEFORE
style.4295bfab83c8c27439dd.css    29.9 kB       0  [emitted]  main

// AFTER
style.7bd1e9e21d4a48ece104.css    43.3 kB       0  [emitted]  main

Proposal was made do Vanilla to improve that: https://github.com/vanilla-framework/vanilla-framework/issues/1147

bartaz commented 7 years ago

As discussed with @nottrobin we can workaround some of these issues by creating a 'custom' button component that uses both Vanilla styles and additional functionalities (such as spinner).

matthewpaulthomas commented 7 years ago

2. Vanilla buttons have transition on hover

Hover effects are needless noise for anything that already looks interactive, and animated hover effects are even worse. But I didn’t say anything about that until now, and I’d be surprised if anyone else deliberately made BSI’s hover effects different from Vanilla’s. Aligning with Vanilla here would at least fix the small bug that “Add” has a hover effect even when it’s disabled.

3. BSI buttons have bigger/smaller options

We have buttons in different sizes from Vanilla. We have bigger buttons on landing page:

It’s common for sign-up / get-started buttons (and text fields and other controls, too) on account-essential sites to be bigger than usual: see for example facebook.com, outlook.com, or wordpress.com. I think it’s quite appropriate for our front page to do the same. Maybe it should even be part of the Brochure theme.

and a bit smaller buttons in "Any missing" dialog:

I’d have no problem with dropping the “smaller” size for the “Any missing?” buttons; that way they’d be more consistent with the “Cancel” and “Add” buttons. However, this would require a change in Vanilla. Outside of dialogs — and in this case, even inside a dialog — it’s common for buttons to be alongside text fields. Therefore, to line up, the button height should equal the text field height. The missing-repos explanation uses 24+6+6 = 36px for both. Unfortunately, Vanilla defaults to 16+12+12 = 40px for buttons and 24+12+12 = 48px for fields. So that would need harmonizing.

5. BSI buttons have spinner inside, that requires additional styles

I’d be interested to see how other Vanilla-using apps provide reassurance during slow submissions. If the answer is “not at all, yet”, then maybe this should be part of Vanilla.

6. Vanilla base button hover color is not visible in dropdowns

I guess there are three possible solutions: (a) remove Vanilla’s hover effects (which would require retiring “base buttons” altogether), (b) change Vanilla’s base button hover color, or (c) change our expanded-row color. I’d prefer (a), but don’t really mind which, as long as expanded and unexpanded rows remain clearly distinct.

nottrobin commented 7 years ago
  1. Our current button size is a bit different from Vanilla Vanilla buttons seem to be a bit bigger (more padding) than current BSI buttons. It's not a huge issue, but a bit visible - especially if we only convert some of the buttons, not all of them.

I think we should convert all of them, and change the button size to Vanilla's button size.

  1. Vanilla buttons have transition on hover

Aligning with Vanilla here would at least fix the small bug that “Add” has a hover effect even when it’s disabled.

Agreed, aligning with Vanilla seems like the solution here

  1. BSI buttons have bigger/smaller options

We have buttons in different sizes from Vanilla. We have bigger buttons on landing page: It’s common for sign-up / get-started buttons (and text fields and other controls, too) on account-essential sites to be bigger than usual: see for example facebook.com, outlook.com, or wordpress.com. I think it’s quite appropriate for our front page to do the same. Maybe it should even be part of the Brochure theme.

Yes, we should just make a local modification for build.snapcraft.io to support these larger buttons, and we should suggest it to the vanilla-design repo. Although I suspect they might say that until another site needs / wants bigger buttons it shouldn't be part of the framework.

I’d have no problem with dropping the “smaller” size for the “Any missing?” buttons; that way they’d be more consistent with the “Cancel” and “Add” buttons. However, this would require a change in Vanilla. Outside of dialogs — and in this case, even inside a dialog — it’s common for buttons to be alongside text fields. Therefore, to line up, the button height should equal the text field height. The missing-repos explanation uses 24+6+6 = 36px for both. Unfortunately, Vanilla defaults to 16+12+12 = 40px for buttons and 24+12+12 = 48px for fields. So that would need harmonizing.

Could we just use the standard Vanilla buttons - and then change the height of the input fields to match? We should definitely make the point that Vanilla should harmonise on height, but in the mean-time, since we're not actually using the Vanilla patterns for our form fields yet, we could maybe just make the fields 40px high for now.

  1. BSI buttons have spinner inside, that requires additional styles

I’d be interested to see how other Vanilla-using apps provide reassurance during slow submissions. If the answer is “not at all, yet”, then maybe this should be part of Vanilla.

I don't think other sites do it yet, but I also don't think it necessarily needs to be part of Vanilla at this stage. It's simply the content of the button, so we should write our own extensions to show the loading image. It would be good to propose it back up to Vanilla to share our work, but it's not the most pressing concern.

  1. Vanilla base button hover color is not visible in dropdowns

I guess there are three possible solutions: (a) remove Vanilla’s hover effects (which would require retiring “base buttons” altogether), (b) change Vanilla’s base button hover color, or (c) change our expanded-row color. I’d prefer (a), but don’t really mind which, as long as expanded and unexpanded rows remain clearly distinct.

I think in other sites, things like this "Cancel" action here are presented as links rather than buttons. Is that an option? If it is at least an option, then implementing the vanilla default so the "cancel" button looks more like a link on hover than a button might not be the worst thing in the world, in the short term.

Ultimately this is definitely a problem that we need to bring up in the Vanilla working group. Vanilla should have recommendations for the clashing colours here.

  1. Vanilla has additional 'is-inline' button modifier It's rather an implementation detail than an issue, but Vanilla has special is-inline modifier class for inline buttons (to give them some margin). We don't have such styles in BSI, but it's probably just a matter of adding support for it and starting to use it when appropriate.

I think it's worth raising this in Vanilla working group. In this case, I don't understand why --inline wouldn't be an explicit modifier to the button pattern (p-button--inline) - since it's clearly a common type of button that one might want. In general, I think a lot of the modifiers are crutches that allow us to escape designing our patterns properly. Of course this is a discussion for Vanilla rather than here.

matthewpaulthomas commented 7 years ago

Notes from meeting:

1. Set our buttons and fields to 40px, then request that Vanilla reduce its field height.

2. Align hover effects with Vanilla.

3. Other sites have larger buttons too, so this should be a pattern.

4. Mention icon-in-button to Vanilla WG in case anyone else does something similar.

5. Discuss spinner-in-button with Vanilla WG to plan long-term harmonization.

6a. Go ahead with the Vanilla-ization anyway, temporarily accepting that it stops looking like a button on hover.

6b. Apparently not an issue, since Vanilla buttons use inline-block.

7. Partly solved by Rich, and the rest is not a blocker.

carlaberkers commented 7 years ago

As discussed and agreed in meeting:

  1. Make buttons Vanilla height and make fields same height as button. Start discussion in Vanilla to make forms and buttons match.

  2. Follow Vanilla.

  3. Implement larger buttons locally and propose larger button pattern for Vanilla (keeping Juju, MAAS, and other products into account). "large" being an option on the React component, but on the BEM/Vanilla level it will be an explicit type of button - its own sub-pattern. Smaller buttons can become Vanilla size

  4. Keep as is. We can propose this to Vanilla as a pattern - see if others might like to use it.

  5. Make spinner button consistent with MAAS and Juju solutions. Push upstream to Vanilla.

  6. Change to Vanilla buttons and don't worry about hover. We will fix the issue when we convert tables to Vanilla.

second 6. Ignore for now.

  1. Already raised in Vanilla so don't worry. Keep an eye on it and keep pushing Vanilla to be better.
matthewpaulthomas commented 7 years ago

3.…

Unfortunately, Vanilla defaults to 16+12+12 = 40px for buttons and 24+12+12 = 48px for fields.

I don’t know whether this has changed in the past ten days, or (more likely) I mismeasured, but now they both seem to be 42px. (It’s radio menus that are inconsistent.) So this is no longer an obstacle.

5.…

I’d be interested to see how other Vanilla-using apps provide reassurance during slow submissions. If the answer is “not at all, yet”, then maybe this should be part of Vanilla.

I don't think other sites do it yet, but I also don't think it necessarily needs to be part of Vanilla at this stage. It's simply the content of the button, so we should write our own extensions to show the loading image.

I suggested that it be part of Vanilla to ensure consistency in details like (a) the button becoming disabled after it is clicked, (b) the flicker-reducing timeout before the text is replaced by the spinner, and (c) that replacement not altering the width of the button. The argument against was that Vanilla is about style, rather than behavior, which is understandable. However, vanilla-design#29 calls for “more animation” in Vanilla, and vanilla-design#13 specifically proposes a spinner inside a button as a demo of the spinner.