Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

source{d} branded superset UI - Typography #106

Merged
merged 6 commits into from
Jun 14, 2019

Conversation

ricardobaeta
Copy link
Contributor

@ricardobaeta ricardobaeta commented Jun 13, 2019

This PR is related to the 2019-R5-CE and addresses the source{d}-branded superset UI EPIC.

Our approach is to address initially the overall Typography on the UI, and the state of this PR it's only related to Typography, and nothing else on the UI. Keep that in mind when reviewing. Our brand typography "Source Sans Pro" & "Source Code Pro" were applied.

This approach is part of the source{d}-branded Superset UI Design Document on the Typography Solution - where you can check the rationale of the decisions made.

The code on the custom-brand.less is still immature, given the issues I'm having on redefining variables, and them not being applied to the UI, because they're overwritten by the default ones. Solving this issue would be a priority, IMHO, to get the code clean without !important's all over and not get the most out of variables. Also, this is an incremental evolution, thus the Typography work is not perfect yet.

A codemirror_override.less file was added to address the UAST code panel, where we overwrite rules taken from the solarized codemirror theme and the following brand work will evolve on mentioned file.

This PR will evolve to continue to address the Solutions on the mentioned Design Doc, and the next steps are going to be:

  1. UI Components Solution
  2. UI Interactive Elements Solution

We'll tackle the remaining Solutions for this source{d}-branded superset UI EPIC, right after.

@ricardobaeta ricardobaeta changed the title Sourced branded superset UI source{d} branded superset UI Jun 13, 2019
@smacker
Copy link
Contributor

smacker commented Jun 13, 2019

Please also remove .DS_Store file. To avoid adding them in the future you can create file ~/.gitignore and add .DS_Store there. Then git will always ignore it.

@smacker
Copy link
Contributor

smacker commented Jun 13, 2019

I'll test it manually when comments from above are applied.

@ricardobaeta
Copy link
Contributor Author

All comments addressed! Thank you so much @smacker :)

@smacker
Copy link
Contributor

smacker commented Jun 13, 2019

Change of the font size broke table charts:
Screenshot 2019-06-13 at 15 07 41

Screenshot 2019-06-13 at 15 09 15

@smacker
Copy link
Contributor

smacker commented Jun 13, 2019

correct font wasn't applied:

Screenshot 2019-06-13 at 15 10 05

@smacker
Copy link
Contributor

smacker commented Jun 13, 2019

font-face defenition isn't correct most probably:

@font-face {
  font-family: "Source Sans Pro", "Helvetica Neue", Arial, sans-serif;
  src: url("../fonts/SourceSansPro-Regular.ttf") format("truetype");
}

As far as I know only 1 font in font family can be defined.
https://developer.mozilla.org/en-US/docs/Web/CSS/@font-face

@ricardobaeta
Copy link
Contributor Author

Change of the font size broke table charts:

Yes, this is typography work, and it has dependencies, and one of them is addressing our design doc UI Elements Solution.

Will keep pushing forward!

@ricardobaeta
Copy link
Contributor Author

correct font wasn't applied:

Yes, as you say we can't have two font-face declarations. And we're calling Source Sans Pro with font-face and it's ok. We need to find a solution for calling Source Code Pro to apply it to the code on the UI.

Suggestions?

@smacker
Copy link
Contributor

smacker commented Jun 13, 2019

After all the fixes tested locally and it's good enough to go. There are some minor problems but original UI also have problems so I don't think we should block the PR because of it.
The only "major" problem is broken dashboard but we can address it in a separate PR (by adjusting the size of the widgets and re-exporting json)

The only thing I would still improve is to rebase commits. But it might be a little bit too complicated for Ricardo, so I'll do it myself later and re-push into this branch.

I also create an issue for a small improvement we can do later: #108

@ricardobaeta
Copy link
Contributor Author

ricardobaeta commented Jun 14, 2019

I also create an issue for a small improvement we can do later: #108

@smacker This is solved by this commit.

@marnovo marnovo self-requested a review June 14, 2019 11:23
Copy link
Member

@marnovo marnovo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks Ricardo. I failed to run it locally, can I trust typography is consistent across the interface elements?

If so, agreed with @smacker on #106 (comment). Let's merge typography and rebase + quick fix the dashboards afterwards.

@ricardobaeta
Copy link
Contributor Author

ricardobaeta commented Jun 14, 2019

Thanks Ricardo. I failed to run it locally, can I trust typography is consistent across the interface elements?

Source Sans Pro & Source Code Pro is applied and controlled across the typographic interface elements. We still aren't making the most out of variables - the proper way to be consistent - therefore, we might still find some outliers and UI elements affected by hacky typography code work.

Sorting these variables issue has, in my opinion, a strong priority to comprehensively control the consistency and hierarchy of typography embedded on all interface elements.

If so, agreed with @smacker on #106 (comment). Let's merge typography and rebase + quick fix the dashboards afterwards.

I agree with @smacker as well.

@marnovo marnovo changed the title source{d} branded superset UI source{d} branded superset UI - Typography Jun 14, 2019
@smacker smacker force-pushed the sourced-branded-superset-ui branch from 289dc67 to 2730901 Compare June 14, 2019 12:33
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.

Signed-off-by: Ricardo Baeta <[email protected]>
Signed-off-by: Maxim Sukharev <[email protected]>
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

Signed-off-by: Ricardo Baeta <[email protected]>
Signed-off-by: Maxim Sukharev <[email protected]>
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.

Signed-off-by: Ricardo Baeta <[email protected]>
Signed-off-by: Maxim Sukharev <[email protected]>
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

Signed-off-by: Ricardo Baeta <[email protected]>
Signed-off-by: Maxim Sukharev <[email protected]>
This commit adresses the issues pointed out early on by Maxim about code formating. It also adresses a couple more typography outliers elements branding.

Signed-off-by: Ricardo Baeta <[email protected]>
Signed-off-by: Maxim Sukharev <[email protected]>
@carlosms
Copy link
Contributor

LGTM. Although the new typography changes the height of some elements and there are places where this might not fit.
See for example the top right "edit dashboard" drop down. Or the chevron-down icon next to the language flag.

Before:
127 0 0 1_8088_superset_dashboard_1_ (1)

After:
127 0 0 1_8088_superset_dashboard_1_

This commit replaces font .ttf files for .woff2 and the related changes on the less files.

Signed-off-by: Ricardo Baeta <[email protected]>
Signed-off-by: Maxim Sukharev <[email protected]>
@smacker smacker force-pushed the sourced-branded-superset-ui branch from 2730901 to 0414d65 Compare June 14, 2019 12:39
@smacker
Copy link
Contributor

smacker commented Jun 14, 2019

@carlosms sorry we were discussing it on slack. Ricardo is aware of that small issues and they will be fixed in next prs. Those things are the

are some minor problems

which I mentioned in my prev comment

@ricardobaeta
Copy link
Contributor Author

Thanks @carlosms !

LGTM. Although the new typography changes the height of some elements and there are places where this might not fit.
See for example the top right "edit dashboard" drop down. Or the chevron-down icon next to the language flag.

Yes, these are known issues, that we'll be solved in the upcoming PR's.

@smacker smacker merged commit c18453f into master Jun 14, 2019
@carlosms carlosms deleted the sourced-branded-superset-ui branch June 14, 2019 14:01
carlosms added a commit to carlosms/sourced-ui that referenced this pull request Jun 17, 2019
…set-ui"

This reverts commit c18453f, reversing
changes made to eecb4c8.
@carlosms carlosms mentioned this pull request Jun 17, 2019
carlosms added a commit to carlosms/sourced-ui that referenced this pull request Jun 17, 2019
…set-ui"

This reverts commit c18453f, reversing
changes made to eecb4c8.

Signed-off-by: Carlos Martín <[email protected]>
carlosms added a commit that referenced this pull request Jun 17, 2019
@dpordomingo dpordomingo mentioned this pull request Jun 20, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants