Branding typography #139

dpordomingo · 2019-06-20T06:48:58Z

fix #134
fix #125
fix #124
fix #123
fix #122
fix #114

The new typography was introduced by #106, but some days later it was reverted by #128, so its changes were rollbacked.

Original changes made by #106 were: 555765c 450de62 43ba635 a20fde3 ec5d9ed 0414d65

After being reverted, an extra commit was applied on top of those ones: abb3431 to address the problems described by #125 #124 #123 #122

As requested by #134, I took the old work and applied on top of the current master in order to validate and merge it.

click to see more details of how the previous commits were rearranged on top of `master`

1. I took all the old six commits from above and rebase-squashed them into: d8b774b
2. And then applied ~~abb3431~~ → c828320 on top, to fix the described problems.
3. Create this PR with the new two commits, containing the old and outdated commits.

Some screenshots testing it over the current sourced v0.14.0-beta.5

Old view:

New view:

New collaboration dashboard:

dpordomingo · 2019-06-20T06:50:38Z

Let me know if I should do any other testing, please.

smacker · 2019-06-20T08:19:36Z

plz this one as well #113
also try to click on dropdowns and other stuff on charts creation page to makes sure nothing is broken there

smacker · 2019-06-20T08:22:15Z

according to screenshot, #114 and #118 aren't fixed, right?

ricardobaeta

👍 Thank you David!

ricardobaeta · 2019-06-20T09:16:40Z

according to screenshot, #114 and #118 aren't fixed, right?

No they aren't @smacker , we talked about this and we agreed that chart fixing with be easily taken care by you. Can you confirm, please?

plz this one as well #113

Should I be the one to tackle this? Please confirm.

smacker · 2019-06-20T09:52:31Z

@ricardobaeta
as it is described in #114 apparently it isn't easily fixable on our side, my initial guess was wrong.

#113 is important. As I described in the comment it prevents modifying/creating a chart with a long title. And it's not always possible (for example on my laptop) to make the browser window bigger to workaround the problem. In pre-redesign, similar issue exists but much less bad. The chart also gets moved down but x-axis is still visible.

ricardobaeta · 2019-06-20T13:31:26Z

@smacker please have in mind @dpordomingo comment on #113.

smacker · 2019-06-20T13:47:03Z

David said exactly the same what I did in the comment above but in more words:

bug exists in master too
solution is Long title s...

dpordomingo · 2019-06-20T13:51:31Z

I think we should not block the typography redesign by issues that are already happening in current master or are not caused by that redesign itself.

Considering that:

My proposal for this PR:

Test it deeply with the latest sourced-ce and our two available dashboards.
Confirm that already known regressions are solved:
- Tables overflow the chart container #114 (Tables overflow the chart container) → already addressed by 2e1a31e
- Top menu looks incorrect #125 (Top menu looks incorrect) → already addressed by c828320
- Close button isn't clickable on sqllab params #124 (Close button is not clickable) → already addressed by c828320
- Table headers look incorrect #123 (Incorrect table headers) → already addressed by c828320
- Buttons looks incorrent #122 (Buttons looks like wrong) → already addressed by c828320
Perform an extra sanity check
- dropdowns, chart creation section, uast... and such
Merge once (1), (2), (3) are ready
Do not block this PR for non-regressions: Iterate later
- On chart edit page a chart isn't visible fully #113 (On chart edit page a chart isn't visible fully) → not caused by the redesign, more details, and possible workaround at On chart edit page a chart isn't visible fully #113 (comment)
- Missing summary chart isn't visible #118 (Summary chart overflow) → not a css issue but a dashboard layout issue (solved by Fix "summary chart" height #129).

marnovo · 2019-06-20T16:06:18Z

@dpordomingo agreed. cc: @smola

smacker · 2019-06-25T13:36:48Z

Let's rebase on master and check with new charts. For the rest looks like everybody agrees it's good to go already and we can merge.

dpordomingo · 2019-06-27T06:07:05Z

I rebased over current master,
built a new image and tried it with new sourced,
confirmed that all blockers described by (2) at Branding typography #139 (comment) are still solved,
quick sanity check (3) confirming that there are not huge outages in:
- our current 3 dashboards,
- chart creation,
- dashboard creation and reordering charts
- running queries in SQL Lab
- viewing UASTs

imo this is ready again to be considered as good to go.

dpordomingo · 2019-06-27T06:25:37Z

Since #142 was made on top of this one, you might prefer to deeply test the other one, and approve this one if #142 pass your tests :D

smacker

Tested locally looks amazing.

Syntax highlighting in UAST tab doesn't work in Firefox though:

but we can fix it later

dpordomingo · 2019-06-27T10:14:00Z

I leave here, per @smacker requirement, some screenshots showing how blocker issues were solved, and how the new UI looks like if this and #142 are merged.

#125 (Top menu looks incorrect)
#122 (Buttons looks like wrong)

#124 (popups with not clickable close button)

#123 (Incorrect table headers)

Collaborative dashboard

Overview dashboard
#114 (Tables overflow the chart container)

Welcome dashboard

Chart creation

Bblfsh tab

SQL Lab

carlosms · 2019-06-28T09:27:14Z

I see the UAST tab changes some of the global styles. I have only noticed it in the top-right "New" button, it gets bigger. But there might be other places:

carlosms · 2019-06-28T09:28:26Z

Why don't we fix #118 in this PR too @dpordomingo? I mean editing the chart height in the dashboard json.

carlosms · 2019-06-28T09:37:04Z

I've noticed the sql lab results table now has an extra outer scrollbar. See before:

And after:

carlosms · 2019-06-28T09:41:37Z

I've found a missing change in button height. In this case it's a minor thing, but it means there might be other places where buttons are not consistent.

Before:

After:

marnovo · 2019-06-28T09:55:55Z

Looking great. :)

Regarding the inconsistencies at @carlosms has shown… @dpordomingo @ricardobaeta would this be something that we would fix/avoid by figuring out the puzzle with the "global variables" (i.e. being able to properly set/propagate styles top-down) first?

smacker · 2019-06-28T10:02:57Z

jfyi

extra outer scrollbar

I experienced this before (not sure if with current sourced-ui or the one based on previous superset 0.32)

carlosms · 2019-06-28T14:15:05Z

Also as can be seen in the previous comment #139 (comment), the UAST button inside the results table now overlaps one row with the next one, and does not look as neat.

dpordomingo · 2019-06-29T06:55:39Z

Do we want to release the branding once we can guarantee that it has 0 errors? or do we assume that there can be small issues to be solved in an iterative process?

I understood that it was accepted that some parts could be addressed separately as long as it would not impact heavily in the UX. I assume that we considered it because it was agreed that the benefits of having a rebranded site, are higher than the costs of having some dangling elements, pending to be adapted.

Do the arisen issues cause more damage, than the benefit of the rebranding as this PR and #142 propose?

For sure I'm not saying that we should not fix any small problem we find, nor they're not important, nor we should not deeply review the design. What I think is that we should consider if those issues impact more to the user than the benefit of merging and then continue improving in shorter iterations.

Once said that, let's consider each reported issue:

UAST tab changes some global styles. #183 (UAST tab changes some global styles)
UAST buttons in result tables has no margins #186 (UAST buttons in result tables has no margins)
Missing summary chart isn't visible #118 (Summary chart overflow)
- we could change dashboards charts in this PR, but I think it would be better if done in a separate PR Increase files missing chart height #187. Why? I think it's better to separate theming PR from dashboards (e.g. since this PR increase the fonts, we could also consider increasing other charts, and not only this one, like proposed by Charts disalignment in Collaboration dashboard #162)
Extra outer scrollbar in SQL lab results #184 (Extra outer scrollbar in SQL lab results)
- that problem already exists in srcd/sourced-ui:v0.3.0-rc.1 as described in the issue.
Buttons does not keep size on action #185 (Buttons does not keep the size on action)
- the problem appears now in two axes, but it was previously happening in one because x-axis should not change on action.

Should we block this PR until we deeply review this redesign to ensure there is no other issue like the ones reported above? Are we going to exhaustively review all the PRs addressing the rebranding? Or are we only doing sanity checks before merging, and considering the rebranding as something that will be done iteratively?

This initial commit conveys the font-family, weight, size, spacing, hierarchy and overall combination of these typography characteristics across the entire UI. 1) "fonts" directory containing Source Sans Pro & Source Code Pro font files was created. (not sure if we need to have the entire family files) 2) I'm having troubles yet with variables on custom-brand.less, becasue they don't seem to overwrite default ones. (would be great to really get the most out of it) 3) We'll need to address the typography on UI elements that are not covered in the typography section "less structure" given by superset by default. (will add rules to convey every typography need) 4) We need to create a new less file "codemirror_override.less" to brand the uast code & uast view. Typography outliers elements This commit conveys item 3) from the initial commit: We'll need to address the typography on UI elements that are not covered in the typography section "less structure" given by superset by default. 1) There are still elements that are brand outliers, but will be taken care of on the next commit. 2) Will focus now on the codemirrror less Applies brand typography to outliner elements This commit continues to address the outlier elements that are on the UI that are not covered by the typography less section variables & rules that supertset has by default. Adds file & rules to override codemirror styles In this commit a file was added "codemirror_override.less" that enables us to change the font of the left panel to Source Code Pro. 1) Colour scheme will be tackled later Replaces Typography .ttf files for .woff2 Signed-off-by: Ricardo Baeta <[email protected]> Signed-off-by: Maxim Sukharev <[email protected]> Signed-off-by: David Pordomingo <[email protected]>

This commit contains typography fixes that solve: 1) src-d#125 2) src-d#124 3) src-d#123 4) src-d#122 It also contains the initial code for the Navbar + Navbar Links & Icons + Navbar CTA, and Navbar Dropdown branding. Signed-off-by: Ricardo Baeta <[email protected]> Signed-off-by: David Pordomingo <[email protected]>

This commit will solve issue src-d#114. Signed-off-by: Ricardo Baeta <[email protected]>

This commit removes the bottom dash from the dashboard header. Signed-off-by: Ricardo Baeta <[email protected]>

dpordomingo added enhancement New feature or request product-design Visual, branding, typography labels Jun 20, 2019

dpordomingo requested review from smacker, carlosms and ricardobaeta June 20, 2019 06:48

dpordomingo assigned ricardobaeta and dpordomingo Jun 20, 2019

ricardobaeta approved these changes Jun 20, 2019

View reviewed changes

dpordomingo force-pushed the branding-typography branch from b3ff5a0 to c828320 Compare June 20, 2019 08:57

dpordomingo added product-design Visual, branding, typography and removed product-design Visual, branding, typography labels Jun 20, 2019

ricardobaeta mentioned this pull request Jun 20, 2019

Adds UI Elements Branding to UI #142

Merged

dpordomingo force-pushed the branding-typography branch from afd5414 to 7449f18 Compare June 27, 2019 05:03

smacker approved these changes Jun 27, 2019

View reviewed changes

ricardobaeta mentioned this pull request Jun 27, 2019

Welcome Dashboard Title styling on markdown files #174

Closed

marnovo mentioned this pull request Jun 28, 2019

Improve LESS and CSS, test branding branch #132

Open

This was referenced Jun 29, 2019

UAST tab changes some global styles. #183

Closed

Extra outer scrollbar in SQL lab results #184

Closed

Buttons does not keep size on action #185

Closed

UAST buttons in result tables has no margins #186

Closed

Increase files missing chart height #187

Merged

carlosms approved these changes Jul 2, 2019

View reviewed changes

ricardobaeta added 4 commits July 2, 2019 13:20

Fix Tables overflow the chart container (114)

39b6d9c

This commit will solve issue src-d#114. Signed-off-by: Ricardo Baeta <[email protected]>

Fix non interactive header bottom dash bug

158fbae

This commit removes the bottom dash from the dashboard header. Signed-off-by: Ricardo Baeta <[email protected]>

dpordomingo force-pushed the branding-typography branch from 7449f18 to 158fbae Compare July 2, 2019 11:22

dpordomingo merged commit f8359b3 into src-d:master Jul 2, 2019

dpordomingo deleted the branding-typography branch July 2, 2019 12:10

ricardobaeta mentioned this pull request Jul 4, 2019

Typography fixes for merged Branding PR's #196

Merged

carlosms mentioned this pull request Jul 15, 2019

Fix hidden x-scrollbar #210

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Branding typography #139

Branding typography #139

dpordomingo commented Jun 20, 2019 •

edited

Loading

dpordomingo commented Jun 20, 2019

smacker commented Jun 20, 2019

smacker commented Jun 20, 2019

ricardobaeta left a comment

ricardobaeta commented Jun 20, 2019 •

edited

Loading

smacker commented Jun 20, 2019

ricardobaeta commented Jun 20, 2019

smacker commented Jun 20, 2019

dpordomingo commented Jun 20, 2019 •

edited

Loading

marnovo commented Jun 20, 2019 •

edited

Loading

smacker commented Jun 25, 2019

dpordomingo commented Jun 27, 2019 •

edited

Loading

dpordomingo commented Jun 27, 2019

smacker left a comment •

edited

Loading

dpordomingo commented Jun 27, 2019

carlosms commented Jun 28, 2019

carlosms commented Jun 28, 2019

carlosms commented Jun 28, 2019

carlosms commented Jun 28, 2019

marnovo commented Jun 28, 2019

smacker commented Jun 28, 2019

carlosms commented Jun 28, 2019

dpordomingo commented Jun 29, 2019 •

edited

Loading

Branding typography #139

Branding typography #139

Conversation

dpordomingo commented Jun 20, 2019 • edited Loading

dpordomingo commented Jun 20, 2019

smacker commented Jun 20, 2019

smacker commented Jun 20, 2019

ricardobaeta left a comment

Choose a reason for hiding this comment

ricardobaeta commented Jun 20, 2019 • edited Loading

smacker commented Jun 20, 2019

ricardobaeta commented Jun 20, 2019

smacker commented Jun 20, 2019

dpordomingo commented Jun 20, 2019 • edited Loading

marnovo commented Jun 20, 2019 • edited Loading

smacker commented Jun 25, 2019

dpordomingo commented Jun 27, 2019 • edited Loading

dpordomingo commented Jun 27, 2019

smacker left a comment • edited Loading

Choose a reason for hiding this comment

dpordomingo commented Jun 27, 2019

carlosms commented Jun 28, 2019

carlosms commented Jun 28, 2019

carlosms commented Jun 28, 2019

carlosms commented Jun 28, 2019

marnovo commented Jun 28, 2019

smacker commented Jun 28, 2019

carlosms commented Jun 28, 2019

dpordomingo commented Jun 29, 2019 • edited Loading

dpordomingo commented Jun 20, 2019 •

edited

Loading

ricardobaeta commented Jun 20, 2019 •

edited

Loading

dpordomingo commented Jun 20, 2019 •

edited

Loading

marnovo commented Jun 20, 2019 •

edited

Loading

dpordomingo commented Jun 27, 2019 •

edited

Loading

smacker left a comment •

edited

Loading

dpordomingo commented Jun 29, 2019 •

edited

Loading