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

Icon, font, color improvements #40

Merged
merged 26 commits into from
Apr 22, 2020
Merged

Icon, font, color improvements #40

merged 26 commits into from
Apr 22, 2020

Conversation

jessicaschilling
Copy link
Contributor

@jessicaschilling jessicaschilling commented Apr 13, 2020

Proposing this as major release for ipfs-css; fixes a number of outstanding issues.

Closes #21

  • Includes icon names underneath all icons in icon reference document
  • Renames stroke_picture.svg to stroke_doc_picture.svg for consistency (NOTE: Will need to indicate this in release notes, though a code search in all repos on GitHub doesn't indicate this being anyone's breaking change)

Closes #28

  • Adds all remaining missing icons

Unblocks (but doesn't close) ipfs/ipfs-webui#1379

  • Replaces teal value #3e9096 with #378085 for accessibility as proposed here

Augments (already-closed) #37

  • Adds support for SCSS via community contributor PR feat: support scss #39 (thanks, @bluelovers!)
  • Builds on that PR by generating its formerly static vars.scss and theme.scss from theme.json
  • Moves both vars.scss and theme.scss up to top level of directory structure, so it's in the same place as ipfs.css

Closes #29 - but with question

  • Updates to latest version of Montserrat from https://github.com/JulietaUla/Montserrat
  • Updates to latest version of Inter from https://github.com/rsms/inter
  • Renames all interui, inter ui, inter ui references to just inter due to 2019 name change
  • Adds @font-face support for the following:
    • Inter: ExtraLight 300, ExtraLightItalic 300, SemiBold 600, SemiBoldItalic 600, ExtraBold 800, ExtraBoldItalic 800
    • Montserrat: Medium 500, MediumItalic 500, ExtraBold 800, ExtraBoldItalic 800
  • Excludes various unused fonts from build:
    • Montserrat: Thin, ThinItalic, ExtraLight, ExtraLightItalic, Black, BlackItalic
    • Inter: Black, BlackItalic, Light, LightItalic, InterDsplay*, Inter-Thin*
  • Qestion: It looks as though in the past we've simply duplicated the entire contents of the Montserrat and Inter repos rather than only bring in the finished font files themselves; have done that in this PR, but I'm wondering why.

Closes #10

  • Un-excludes woff, eot, otf formats when building fonts directory
  • Adds @font-face support for .woff and .eot fonts in Montserrat
  • Adds @font-face support for .woff and .otf fonts in Inter

Miscellany

  • Minor typo/consistency fixes in readme
  • Mentions SCSS in readme
  • Adds maintainer info to readme

- Includes icon names underneath all icons
- Restores to alphabetical order in all sets
- Renames stroke_picture.svg to stroke_doc_picture.svg for consistency (this will need to be noted in release, could be breaking change)
- Indicates missing icons in all views, not just compare view
@jessicaschilling jessicaschilling self-assigned this Apr 13, 2020
@jessicaschilling jessicaschilling changed the title Includes icon names, plus [WIP] Improves icon sheet, adds missing icons Apr 13, 2020
More to come
Full icon set now available in both styles
@jessicaschilling jessicaschilling changed the title [WIP] Improves icon sheet, adds missing icons Improves icon sheet, adds missing icons Apr 14, 2020
@jessicaschilling jessicaschilling changed the title Improves icon sheet, adds missing icons [WIP] Improves icon sheet, adds missing icons Apr 15, 2020
@jessicaschilling jessicaschilling changed the title [WIP] Improves icon sheet, adds missing icons [WIP] Icon, font, color improvements Apr 15, 2020
Replaces src/fonts/inter-ui and src/fonts/montserrat with the latest from master branch of their respective GitHub repos. Question though: why are we bringing in all of these assets?
@jessicaschilling jessicaschilling changed the title [WIP] Icon, font, color improvements Icon, font, color improvements Apr 16, 2020
@jessicaschilling
Copy link
Contributor Author

@olizilla, @hacdias and/or @lidel ... would greatly appreciate your eyes on this one.

  • First, apologies for the single mega-PR. This got a little out of hand. Hopefully the commit history is granular enough to be helpful in this regard.
  • Second, suggestions for the best means of testing this?

Thanks all!

Copy link
Member

@hacdias hacdias left a comment

Choose a reason for hiding this comment

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

Other than my specific comments, I appreciate all the work done here and the icons are much more organized.

Regarding the testing question: I'm not aware on how to test this.

About why we duplicated the fonts repositories: I don't know either. However, it makes more sense to just grab the final fonts.

build/icons-page.js Outdated Show resolved Hide resolved
@olizilla
Copy link
Member

not got a full review, just wanted to clarify:

It looks as though in the past we've simply duplicated the entire contents of the Montserrat and Inter repos rather than only bring in the finished font files themselves; have done that in this PR, but I'm wondering why.

The fonts were included as a git subtree... (https://www.atlassian.com/git/tutorials/git-subtree) for maximum traceability and history preservation, which is nice. We dont have to do that, but it would be nice to continue if it doesn't cause issue.

@jessicaschilling
Copy link
Contributor Author

@olizilla Thanks for the subtree info -- set that back up again.

License: MIT
Signed-off-by: Marcin Rataj <[email protected]>
Copy link
Member

@lidel lidel left a comment

Choose a reason for hiding this comment

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

LGTM: I've smoke-tested his PR against ipfs-companion and ipfs-webui,
did not see any obvious regressions.

Two notes:

  • would it be ok to do a major version bump from 0.13.1 to 1.0.0, just to be sure we don't break anything for anyone using liberal semver ranges?
  • I don't think all "unused" fonts are excluded as noted in the first comment of this PR, fonts/ still include:
    Inter-UI-BlackItalic.woff2                                                                                                                                                                                        
    Inter-UI-Black.woff2                                                                                                                                                                                              
    Inter-UI-LICENSE.txt                                                                                                                                                                                              
    Montserrat-BlackItalic.woff2                                                                                                                                                                                      
    Montserrat-Black.woff2                                                                                                                                                                                            
    Montserrat-ExtraBoldItalic.woff2                                                                                                                                                                                  
    Montserrat-ExtraBold.woff2                                                                                                                                                                                        
    Montserrat-ExtraLightItalic.woff2                                                                                                                                                                                 
    Montserrat-ExtraLight.woff2                                                                                                                                                                                       
    Montserrat-LICENSE.txt                                                                                                                                                                                            
    Montserrat-MediumItalic.woff2                                                                                                                                                                                     
    Montserrat-Medium.woff2                                                                                                                                                                                           
    Montserrat-ThinItalic.woff2                                                                                                                                                                                       
    Montserrat-Thin.woff2  
    
    Is that on purpose? (those unused files are over 1MB in size)

@jessicaschilling
Copy link
Contributor Author

@lidel - good call on the major version. Will do.

As for the extraneous files:

  • All the Inter-UI files shouldn't be there; that's the old name for the Inter font and I'm worrying there might be something screwy happening subtree-wise.
  • The Montserrat-Black and Montserrat-Thin variants shouldn't be there either; they're explicitly being excluded in package.json (again, worried about subtree)
  • Montserrat-ExtraBold, Montserrat-Medium and Montserrat-ExtraLight variants are supposed to be there so that we support the full 300-800 weight range for both fonts.
  • Montserrat-LICENSE.txt is supposed to be there.

Do you mind cleaning and rebuilding and seeing what you get?

Copy link
Member

@hacdias hacdias left a comment

Choose a reason for hiding this comment

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

The changes LGTM. I cloned it from scratch without any submodules and couldn't find any trace of Inter UI. Also, I agree with @lidel that we should be better off releasing a major version just to make sure we don't break anything.

Copy link
Member

@lidel lidel left a comment

Choose a reason for hiding this comment

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

I've tried again in a fresh checked out directory and unexpected fonts are gone.
So it was something on my end. I believe its ok to merge.

@jessicaschilling jessicaschilling merged commit fd80319 into master Apr 22, 2020
@jessicaschilling jessicaschilling deleted the issue-21 branch April 22, 2020 17:55
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.

Upgrade font: Inter UI v3 Include icon names on icon page Add multiple font types
4 participants