search

Tradeshift / tradeshift-ui

The Tradeshift UI Library & Framework
https://ui.tradeshift.com
Other
33 stars 45 forks source link

[TopBar] hide method versus mobile breakpoint #94

Closed wiredearp closed 7 years ago

wiredearp commented 7 years ago

You can hide the TopBar with the somewhat undocumented ts.ui.TopBar.hide() method, but then it will remain hidden in mobile breakpoint. This means that the user cannot access the main navigation and will remain trapped in the app forever.

Let's fix this and also add documentation about the hide and show methods (if we don't have that already).

zdlm commented 7 years ago

The hide and show methods we have them already. They work fine. I will document them

wiredearp commented 7 years ago

@zdlm See comment https://github.com/Tradeshift/tradeshift-ui/pull/98#pullrequestreview-14527657, we should make it so that hide and show has no effect in mobile breakpoint.

TimeBomb commented 7 years ago

We currently use the hide method @wiredearp. We use it for the EngagementFlow. I removed it due to noticing a console error, but found we still need it. See: https://github.com/Tradeshift/Apps/pull/3943 for screenshots of before and after (hiding it).

Engagement Flow designs require that the top bar meets specific criteria:

Right now, we have a hack to hide the top bar, seen in the linked PR, that works alright, and lets us custom code the top bar. Let us know when the top bar supports what we need it to, and we can switch over to using it. Until then, please don't remove our ability to fully hide it in all scenarios. Thanks!

wiredearp commented 7 years ago

Quoting from Slack here: To elaborate on our planned strategy when we fix this bug: You can hide() and show() the TopBar but mobile breakpoint will override this (so that you can access the menu). If you don't want a TopBar to appear in the app under any circumstance, you simple remove the <header data-ts="TopBar"> from the HTML so that there really isn't any TopBar at all :beer: