Skip to content
This repository has been archived by the owner on Jan 16, 2022. It is now read-only.

Refactor: Created Reset CSS and added local fonts #240

Merged
merged 15 commits into from
Nov 12, 2019

Conversation

priscilawebdev
Copy link
Contributor

@priscilawebdev priscilawebdev commented Nov 7, 2019

Type: Refactor and Fix

Description:
I noticed an error in the console regarding to a font load. I have fixed it.

I also replaced two dependencies(typeface-roboto and normalize.css) with locally fonts and reset css files :) . I believe that we don't really extra dependencies for these simple things.

Please let me know what do you think. 😉

@priscilawebdev priscilawebdev changed the title Bug/fixed font load Refactor: fixed font load Nov 7, 2019
@priscilawebdev priscilawebdev changed the title Refactor: fixed font load Refactor: Created Reset CSS and added local fonts Nov 7, 2019
@juanpicado juanpicado self-requested a review November 7, 2019 16:40
package.json Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Nov 10, 2019

Codecov Report

Merging #240 into master will increase coverage by 0.05%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #240      +/-   ##
==========================================
+ Coverage   90.25%   90.31%   +0.05%     
==========================================
  Files         139      140       +1     
  Lines         924      929       +5     
  Branches      143      161      +18     
==========================================
+ Hits          834      839       +5     
+ Misses         78       75       -3     
- Partials       12       15       +3
Impacted Files Coverage Δ
src/components/Icon/styles.ts 90% <ø> (ø) ⬆️
src/App/App.tsx 93.87% <100%> (ø) ⬆️
src/design-tokens/ResetStyles.tsx 100% <100%> (ø)
src/design-tokens/StyleBaseline.tsx 100% <100%> (ø)
src/utils/styles/media.ts 81.81% <0%> (ø) ⬆️
src/components/DetailSidebar/DetailSidebar.tsx 85% <0%> (ø) ⬆️

@juanpicado
Copy link
Member

I'm still not agree removing "normalize.css": "8.0.1", and "typeface-roboto": "0.0.75",, frankly I don't see the benefit as we discussed previously.

@verdacciobot
Copy link

Thanks for your PR, the @verdaccio/ui package will be accessible from here for testing purposes:

npm install @verdaccio/[email protected] --registry https://registry.verdaccio.org

Copy link
Contributor

@DanielRuf DanielRuf left a comment

Choose a reason for hiding this comment

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

LGTM

@priscilawebdev priscilawebdev merged commit 09fe1db into master Nov 12, 2019
@delete-merged-branch delete-merged-branch bot deleted the bug/fixed_font_load branch November 12, 2019 07:18
@juanpicado
Copy link
Member

juanpicado commented Nov 12, 2019

Unfortunately this not have my approval, previously we discussed that there reset must comes from dependencies and still are here
https://github.com/verdaccio/ui/pull/240/files#diff-684c724a43a7ac26119a730764a995eeR7

Now we have reset CSS duplicated

import 'normalize.css';
import 'typeface-roboto/index.css';

import ResetCSS from './ResetStyles';

Please rollback those changes.

@juanpicado
Copy link
Member

One more thing, import 'typeface-roboto/index.css'; this is not fully well, if the typeface team changes the main endpoint in their packages, the url won't work, the best is rely on the main field in the package.json, thus import 'typeface-roboto is the best option here.

@DanielRuf
Copy link
Contributor

Afaik there will not be such breaking changes in there. Let's do another PR to resolve this.

And I remember that I had some issue with jest, webpack and so on when I did not specify the .css part.

See https://github.com/KyleAMathews/typefaces/search?q=import+css&type=Issues and especially KyleAMathews/typefaces#113

#134 ;-)

@DanielRuf
Copy link
Contributor

Please rollback those changes

Let's open a new PR for this =)

@priscilawebdev
Copy link
Contributor Author

priscilawebdev commented Nov 12, 2019

@juanpicado there are some css reset things I would like to apply that doesn't come with the normalize.css. Example the font... if you check the current registry.verdaccio.org and inspect some h2 you are going to see that they are not Roboto. That is the reason I kept the ResetStyles component. Regarding the typeface-roboto I just kept it as it was, although I agree with you..

@DanielRuf
Copy link
Contributor

There is a good reason for the .css import (because of jest and so on).

See my last comment.

@juanpicado
Copy link
Member

@juanpicado there are some css reset things I would like to apply that doesn't come with the normalize.css. Example the font... if you check the current registry.verdaccio.org and inspect some h2 you are going to see that they are not Roboto. That is the reason I kept the ResetStyles component. Regarding the typeface-roboto I just kept it as it was, although I agree with you..

I see your point, but I think we have avoid the miss understanding with a better description in your PR about what you were trying to achieve. I'd suggest you rename resetStyles to something more meaningful, as applyFontStyles or something that describe what really does, because reset styles means reset default styles of the browser.

Anyway, I run some test on master and all is ok.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants