Improve LESS and CSS, test branding branch

[carlosms]

This issue is to improve LESS code to make use of variables.
@ricardobaeta please add any other things that need to be reviewed, or that you need assistance with.

[ricardobaeta]

Thanks @carlosms @smacker for this issue. Other than what you described, I don't have that much to add. The variables defined in custom-brand.less get overwritten somehow. If I redefine @font-size-base: for instance, it gets overwritten and I have to declare it on each element.

[dpordomingo]

not a prio for next R6-CE, will resume next week

[marnovo]

Check #139 (comment)

[dpordomingo]

The variables defined in custom-brand.less get overwritten somehow. If I redefine @font-size-base: for instance, it gets overwritten and I have to declare it on each element.
Variables in less are lazily defined, and we have:

superset.less   <<-- entrypoint
  @imports index.less
  @imports vars.less

index.less
  @imports vars.less
  @imports override.less

vars.less
  @brandColor: red

override.less
  @brandColor: blue
  body {
    color: @brandColor
  }

When superset.less is compiled into superset.css, it will appear as it follows:

what would be equivalent to:

superset.less
  @brandColor: red
  @brandColor: blue
  body {
    color: @brandColor
  }
  @brandColor: red

so since vars are defined twice (the second one at the end), that one wins, so no override is done:

superset.css
  body {
    color: red
  }

[dpordomingo]

Here is my diagnostic considering our goal of applying our branding over Apache Superset. I also added some recommendations that I'd like to address in order to enhance the developer experience when rebranding Apache Superset.

note: all routes are relative to superset/superset/assets/

In Apache Superset there are many ways to define the APP styles; they can be summarized in two:

• GLOBAL: src/theme.js, which calls stylesheets/superset.less
• COMPONENTS: each view, component or element, has its own .jsx which loads its own less or css

current limitations:

• Both are independent, and they do not share common variables unless explicitly import them.
• Both define the same kind of properties (layout, cosmetics, typography...).
• Styles applied by COMPONENTS are applied after GLOBAL.

variables definition

About how dimensions, colors, and typographies are defined, we have four situations, that are detailed in each collapsible section below.

TL;DR As it can be seen from the list below, to obtain style consistency, it may be needed either overriding many rules defined by stylesheets that are not using shared variables at all, either modifying those stylesheets to use shared variables.

Using variables

There are two (independent) files defining variables and are used sometimes in some stylesheets

stylesheets/less/cosmo/variables.less defines most of the variables used in GLOBAL, and is also used by some COMPONENTS styles. [see inventory]

• GLOBAL:
  • stylesheets/superset.less
  • stylesheets/less/index.less
• COMPONENTS:
  • src/uast/override.less
  • src/uast/codemirror_override.less
  • src/components/RefreshLabel.less
  • src/SqlLab/main.less

src/dashboard/stylesheets/variables.less defines most of the variables used in dashboard and toast messages; it does not use the variables from the other variables stylesheets even sharing some variable names. [see inventory]

• src/dashboard/stylesheets/index.less
• src/messageToasts/stylesheets/toast.less

Not using shared variables

But there are many stylesheets that do not use variables at all:

many stylesheets use harcoded values instead of using shared variables from above: [see inventory]

• src/chart/chart.css
• src/components/BootstrapSliderWrapper.css
• stylesheets/reactable-pagination.css
• src/components/TableSelector.css
• src/CRUD/styles.css
• src/CRUD/styles.less
• src/explore/main.css
• src/explore/components/controls/DateFilterControl.css
• src/explore/components/controls/VizTypeControl.css
• src/profile/main.css
• src/components/FilterableTable/FilterableTableStyles.css
• src/uast/stylesheets/UASTViewerPane.less
• src/visualizations/FilterBox/FilterBox.css
• src/visualizations/Legend.css
• src/visualizations/PlaySlider.css

some stylesheets do not use shared variables, but —at least currently—, they do not define visual properties: [see inventory]

• src/components/Loading.css
• src/datasource/main.css
• src/explore/components/Control.css
• src/explore/components/controls/CollectionControl.css
• src/uast/stylesheets/UASTModal.less
• src/visualizations/TimeTable/TimeTable.css
• stylesheets/react-select/select.less

[dpordomingo]

Recommendations

• Convert the MAIN stylesheets/less/cosmo/variables.less into the source of shared variables
• Move our own variables into a new CUSTOM variables stylesheet stylesheets/less/cosmo/variables-override.less.
• Make sure that the MAIN variables stylesheet imports our new CUSTOM variables stylesheet to override MAIN values.
  • We should also import and use MAIN variables stylesheet in our custom stylesheet, instead of our CUSTOM one.

The ones above can be done easily by us; the next ones could be done gradually (and are ordered from costless to very hardcore):

• [TOP-FEATURE]; Call our CUSTOM stylesheet, stylesheets/less/custom-brand.less as a complete separate stylesheet at the end of the page (after COMPONENTS stylesheets, instead of before MAIN one). Doing so:
  • [PRO]: we can just override Apache Superset rules, instead of using !important hacks, or playing with CSS specificity.
  • [PRO]: we will get rid of a lot of dupe code in uast stylesheets (e.g. uast/override.less#L38)
• force src/dashboard/stylesheets/variables.less to use the variables from MAIN, so the rest of the components which are already using this other, will actually be using our MAIN variables.
  • there is no clear mapping between vars in MAIN and in src/dashboard/stylesheets/variables.less, even when they share the same name, so they should be considered separately.
• (this will make the update from upstream very difficult) we could also modify the stylesheets not using shared variables, to make them use shared vars.
• plan with Apache Superset team, a strategy to implement in upstream these recommendations, and others also oriented to make the branding easier.

[dpordomingo]

#226 was merged

we could now continue considering the recommendations above

[se7entyse7en]

@dpordomingo what's the status of this? is this waiting for product's approval? is this done and we can open a separate issue for next steps?

[dpordomingo]

I'd consider this as an umbrella taking into account my analysis and my recommendations.

I'm also aware that @ricardobaeta has a pending investigation about how could we integrate into superset some of the improvements that we did, and that would be useful if they were already in upstream.

Alternatives that I see:

• close this issue, open a new one in backlog-apps, and I'll move there my comments.
• close and do nothing, and I'll copy my comments into my todo list.

[se7entyse7en]

I'd prefer to close this one and if you could create a new one with a more specific scope according to your recommendations. 👍 If you do, please close this one.

[dpordomingo]

I'd say this should be considered an epic, taking my message describing the current status as an input, and my recommendations as one proposed strategy to solve the problem in an iterative way.

Would you prefer if I copy-paste both into a new issue and close this?
Or maybe link both in the issue desc, to have the whole history here, and clear details from the very beginning of the thread?

[se7entyse7en]

Maybe the best thing would be to create a new issue as a story in src-d/applications-backlog and then from that we would create all the different subtasks? wdyt? Similarly to what we did for the log level thing.

[dpordomingo]

Since src-d/applications-backlog is a private repo, it would hide all this info, which is something that I dislike because this info is pretty valuable considering sourced-ui as open-source from apache/incubator-superset.

But if you prefer, I can still do it.

[se7entyse7en]

Oh right, sorry, I forgot that it's private. I just thought about it because it's the place where we now put stories. Well, this is something that actually affects all the projects. I'm pretty sure that @rpau already answered me, and that I forgot, but @rpau was there a specific reason for keeping it private?

[dpordomingo]

I agree with moving there, things that are not open source related, like the ones affecting company products, business, strategic decisions... But I'd say this is not the case.

[se7entyse7en]

Well, originally it was supposed also to be a collector of issues for external users AFAIR. Let's keep it in sourced-ui then for now 👍