-
Notifications
You must be signed in to change notification settings - Fork 60
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
feat: remove fonts and icons from distribution package #683
Conversation
Check the deploy log for errors here: https://app.netlify.com/sites/fundamental-styles/deploys Built with commit fed2ad3 https://app.netlify.com/sites/fundamental-styles/deploys/5e4edba1f022310008eb3d4b |
Deploy preview for fundamental-styles ready! Built with commit 1739cf2 |
- title: Type | ||
url: /foundation/type.html | ||
output: web | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed the foundation pages as they are no longer correct (and haven't been for a while)
vertical-align: bottom; | ||
padding: 10px 20px 0; | ||
border-top: solid 1px lightgray; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was related to the foundation pages - only kept what I needed for the font "72" display on "Getting Started"
</div> | ||
<div class="libraries-section__links"> | ||
<a class="libraries-section__more" href="{{site.baseurl}}/getting-started.html" target="_blank">Learn More <span class="vh">about Fundamental Library Styles</span></a> | ||
<a class="libraries-section__more" href="{{site.baseurl}}/getting-started.html">Learn More <span class="vh">about Fundamental Library Styles</span></a> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed target blank here because it's the same domain - shouldn't open in a new tab.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jbadan this was a requirement actually
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why?
|
||
See the [Icon Component]({{site.baseurl}}/components/icon.html) for a list of icon class names. See [Project Configuration](#Project-configuration) below for instructions to include SAP Fiori 3 icons in your project. | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't really have a lot to say about icons - is there a public website about fiori 3 icons like there is for fonts?
for file in public/images/*; do cp "$file" dist/images; done |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We shouldn't be publishing these images either, but that's a separate story.
@@ -54,8 +51,6 @@ $block: #{$fd-namespace}-breadcrumb; | |||
|
|||
display: inline-block; | |||
transition: all $fd-animation--speed ease-in; | |||
font-size: $fd-breadcrumb-font-size; | |||
font-family: $fd-breadcrumb-font-family; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These are both included in fd-reset()
5 lines up.
color: var(--sapField_TextColor); | ||
background-color: var(--sapField_Background); | ||
box-sizing: border-box; | ||
border-style: solid; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All already included in fd-reset()
@@ -39,9 +39,6 @@ $block: #{$fd-namespace}-textarea; | |||
@include fd-reset(); | |||
@include fd-ellipsis(); | |||
|
|||
font-family: var(--sapFontFamily); | |||
font-size: var(--sapFontSize); | |||
font-weight: normal; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All already included in fd-reset()
@@ -106,8 +94,6 @@ li { | |||
<div class="" style="flex: 1;"> | |||
<h2>Specs</h2> | |||
<ul> | |||
<li><a href="pages/styles">Styles</a></li> | |||
<li><a href="pages/text">Text</a></li> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These were outdated - removed for cleanup effort.
Let's merge once we release |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The font on the doc pages is affected
@droshev I had originally only loaded regular 72 to the docs site. Now added Light and Bold so it looks the same. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's go with it
Related Issue
Closes SAP/fundamental-styles#
Description
This pr is showing how to remove the fonts and icons from this project, while still having the docs site and testing playground work appropriately.
TO DO:
[x] add a docs page that explains how to configure your own project.
[x] troubleshoot
fs.copyFile
not working for netlify (but working locally)[x] troubleshoot why visual regression tests are failing
Why are so many tests updated?
Our tests were relying on our locally copied fonts in fundamental-styles. Now that the fonts are being pulled from
sap-themeing
, we've got the updated fonts.