[Footer] Typo in `type` property for a Button triggers unexpected behaviour

tudorminator commented 6 years ago

Bug report

Tradeshift UI version affected

v10.0.25

Expected Behavior

Should create 2 buttons on Footer.

Actual Behavior

Creates a Button Group on Footer, both buttons being of type ts-tertiary as the second button (the one with the correctly spelled type property).

Steps to reproduce

Call ts.ui.Footer.buttons() method with a 2 elements array, as below:

window.ts.ui.Footer.buttons([
  // typo on the next line (it should read `type`)!
  {label : 'Primary', /* here! -> */tytpe : 'ts-primary'},
  {label : 'Tertiary', type : 'ts-tertiary'}
]);

Instead of 2 buttons (one primary and one tertiary), it creates a button group where both buttons are of type ts-tertiary

Reduced test case

https://codepen.io/anon/pen/BVPmQZ

Note: if the second button's type is anything else other than ts-tertiary (so ts-primary or ts-secondary), the above code does create 2 buttons on Footer, where the button with the typo is always of type ts-tertiary.

Tried to reproduce using version 11.0.0-beta.11, but got error Uncaught TypeError: Cannot read property '$inject' of null which results in no footer and no buttons whatsoever. See pen here: https://codepen.io/anon/pen/yEqpOm

wiredearp commented 6 years ago

It's not a really a bug, however, except of course from the TypeError you see in the 11 version [1]. The rule for those buttons is that all primary and secondary buttons are always shown. Tertiary buttons are shown if there is only one. If there is more than one, tertiary will be grouped into a submenu (in the Aside). Furthermore, all buttons that don't have a primary or secondary type are assumed tertiary and this way the tertiary type is optionally declared. This should account for the behavior, since your two buttons would implicitly become tertiary due to the typo.

You could argue that we should validate the input and prevent such typos from happening. Fortunately, that is exactly what we are planning to do in an upcoming revision of the system.

[1] The exception in version 11 could be caused by updating the framework version without also updating the basic page layout (which is different in version 11, see http://ui.tradeshift.com/v11/#getstarted/layout.html). We don't currently validate the page layout and just assume it to be correct, and it is also not always easy to validate, but that is another story.

tudorminator commented 6 years ago

A lot of ifs, apparently. I get it that it's not a bug, but without even a hint from the docs about all those rules, I really had no way of knowing. It had me scratching my head for a while, so I thought I'd better let you know about it.

tudorminator commented 6 years ago

I have updated the v11 pen with the proper layout and the error is gone: https://codepen.io/anon/pen/yEqpOm

wiredearp commented 6 years ago

It is mentioned in the docs, specifically on http://ui.tradeshift.com/v10/#components/bars/topbar.html:

The buttons type property works like the CSS class for a regular Button
The TopBar will automatically sort all buttons from primary to tertiary.
If there"s more than one tertiary button, these will be pushed to an Aside.
In the mobile breakpoint, all the buttons will be pushed to an Aside.

There are however so much docs that information such as this is impossible to find. The section should likely be copied into all pages that mention buttons - or even better, we should have a more abstract page about the behavior of API generated buttons in general, since they behave the same all over the place. The bug would in any case be caught by the code in the upcoming revision that we are working on, I hope this project can also lead to a revision of the docs structure. Thanks for the tip :+1:

Tradeshift / tradeshift-ui