Tradeshift / tradeshift-ui

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

[Bars] CSS classlist not maintained correctly #97

Closed wiredearp closed 7 years ago

wiredearp commented 7 years ago

When bars (such as TabBar and ToolBar) get added and removed, we layout the page via classnames on the html element. There's a bug where these classes are not correctly maintained. Steps to reproduce:

  1. Go to http://ui-dev.tradeshift.com/#components/bars/toolbar.html
  2. Click the green Run This Code to show the ToolBar
  3. Click the tab Inline ToolBar (to hide the ToolBar again)

There's now a hole in the layout where the ToolBar was positioned before we navigated away.

zdlm commented 7 years ago

I try to debug the issue. I found that this.guilayout.shiftGlobal(show, klass); is not working, show is false, klass is ts-has-toolbar-first-ts-macro in ts.ui.ToolBarSpirit.js. The class (ts-has-toolbar-first-ts-macro) would be deleted but not. The function (shiftGloabal) in ts.ui.LayoutPlugin.js is

/**
         * Add or remove classname(s) on the HTML element.
         * @param {truthy} on
         * @param {string|Array<string>} cnames
         * @returns {ts.ui.LayoutPlugin}
         */
        shiftGlobal: chained(function(on, cnames) {
            this.spirit.action.dispatch(ts.ui.ACTION_ROOT_CLASSNAMES, {
                classes: GuiArray.make(cnames),
                enabled: !!on
            });
        }),

@wiredearp What do you think?

wiredearp commented 7 years ago

I think the bug is elsewhere in some code related to "root panels" (which filters the classnames in an extra step).

We should make a test page for root panels before we fix this bug :( although perhaps we have half a test page somewhere, because otherwise we'll break that feature.

On Dec 28, 2016 11:22 AM, "leo zhang" notifications@github.com wrote:

I try to debug the issue. I found that this.guilayout.shiftGlobal(show, klass); is not working, show is false, klass is ts-has-toolbar-first-ts-macro in ts.ui.ToolBarSpirit.js. The class ( ts-has-toolbar-first-ts-macro) would be deleted but not. The function (shiftGloabal) in ts.ui.LayoutPlugin.js is

/**

  • Add or remove classname(s) on the HTML element.
  • @param {truthy} on
  • @param {string|Array} cnames
  • @returns {ts.ui.LayoutPlugin} */ shiftGlobal: chained(function(on, cnames) { this.spirit.action.dispatch(ts.ui.ACTION_ROOT_CLASSNAMES, { classes: GuiArray.make(cnames), enabled: !!on }); }),

@wiredearp https://github.com/wiredearp What do you think?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/Tradeshift/tradeshift-ui/issues/97#issuecomment-269457313, or mute the thread https://github.com/notifications/unsubscribe-auth/AANDbYVZn0u_HbC5Nlw7ezpqX8nNU5zzks5rMjhPgaJpZM4LSuKK .