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

Make ui small again #2270

Merged
merged 21 commits into from
Aug 19, 2022
Merged

Make ui small again #2270

merged 21 commits into from
Aug 19, 2022

Conversation

lookacat
Copy link
Contributor

@lookacat lookacat commented Jul 27, 2022

Description

See owncloud/web#7333

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Technical debt
  • Tests

Checklist:

  • Code changes
  • Unit tests added
  • Acceptance tests added
  • Documentation added/updated

@update-docs
Copy link

update-docs bot commented Jul 27, 2022

Thanks for opening this pull request! The maintainers of this repository would appreciate it if you would create a changelog item based on your changes.

@lookacat lookacat marked this pull request as ready for review August 8, 2022 08:30
src/components/atoms/OcButton/OcButton.vue Outdated Show resolved Hide resolved
src/components/atoms/OcButton/OcButton.vue Outdated Show resolved Hide resolved
src/components/molecules/OcBreadcrumb/OcBreadcrumb.vue Outdated Show resolved Hide resolved
src/styles/theme/oc-text.scss Outdated Show resolved Hide resolved
src/tokens/ods/space.yaml Outdated Show resolved Hide resolved
Copy link
Contributor

@tbsbdr tbsbdr left a comment

Choose a reason for hiding this comment

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

Primary / Secondary Button have different height / (outline border?)
Screenshot 000234@2x

Custom font is not rendered (cause: not bundeled?)
I'd favor to use a more characteristic font that fits to the round/organic ui like the font "Nunito"
Screenshot 000235@2x

@lookacat
Copy link
Contributor Author

lookacat commented Aug 9, 2022

@tbsbdr i have no idea what both issues is causing since i didn't change anything about that :/ Outline border seems to have no effect even on demo.owncloud.com

@tbsbdr
Copy link
Contributor

tbsbdr commented Aug 9, 2022

Regarding the custom font: We have always had the problem with custom fonts - its not embedded in web afaik (and also not loaded via a CDN which is good)

@lookacat lookacat requested review from kulmann and tbsbdr August 9, 2022 11:34
Copy link
Member

@kulmann kulmann left a comment

Choose a reason for hiding this comment

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

More things broken with the primary button style, see button group screenshot (below) from the OcButton docs page.

Screenshot 2022-08-09 at 15 04 30

@tbsbdr
Copy link
Contributor

tbsbdr commented Aug 9, 2022

@lookacat if we ship a Open Font Licence (OFL) -licenced  font, we must at least link (https://scripts.sil.org/OFL) to the OFL Licence

Source: https://scripts.sil.org/cms/scripts/page.php?item_id=OFL-FAQ_web#e71fabc0

Question: 1.10 Does the full OFL license text always need to accompany the font?

Answer: The only situation in which an OFL font can be distributed without the text of the OFL (either in a separate file or in font metadata), is when a font is embedded in a document or bundled within a program. In the case of metadata included within a font, it is legally sufficient to include only a link to the text of the OFL on https://scripts.sil.org/OFL, but we strongly recommend against this. Most modern font formats include metadata fields that will accept the full OFL text, and full inclusion increases the likelihood that users will understand and properly apply the license.

@lookacat lookacat requested a review from kulmann August 15, 2022 09:46
@tbsbdr
Copy link
Contributor

tbsbdr commented Aug 15, 2022

@lookacat just to make sure that you are aware of this - i think we talked about it: Nunito is currently not shown as we don't bundle any custom web fonts - so if you don't have any of the fonts like roboto or nunito already installed on your local system, custom fonts don't show up in ownCloud Web.

how to reproduce

  • disable the respective in font book on mac via: 
     

  • reload local instance
  • Nunito is not rendered

Expected behavior

the respective ownCloud Web font (Nunito) should be included in the App (no ad-hoc download from any public CDN to protect users privacy)

@lookacat lookacat requested a review from tbsbdr August 15, 2022 12:29
@tbsbdr
Copy link
Contributor

tbsbdr commented Aug 15, 2022

almost there ;-)

✅ Font is now rendered, even if its not installed on the local machine

❌ Font is loaded from Google CDN -> GDPR violation, not privacy compliant

Font should be bundeled so that the client does not need to contact a Google CDN

src/components/atoms/OcButton/OcButton.vue Outdated Show resolved Hide resolved
src/components/atoms/OcButton/OcButton.vue Outdated Show resolved Hide resolved
Copy link
Member

@kulmann kulmann left a comment

Choose a reason for hiding this comment

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

You added the font ttf file but you're not embedding it anywhere. Either you use the WebFontLoader with custom option (see docs) or you embed the ttf file and get rid of the WebFontLoader entirely. With this PR freshly pulled and built I see Helvetica on my machine.

@lookacat
Copy link
Contributor Author

embedded it via web via css, but ill try the custom option

@kulmann
Copy link
Member

kulmann commented Aug 16, 2022

embedded it via web via css, but ill try the custom option

Please embed from within ODS.

@@ -1,7 +1,7 @@
---
font:
text:
value: "'Roboto', sans-serif"
face:
Copy link
Member

Choose a reason for hiding this comment

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

FYI, I wanted to use family here, but that didn't show up at all in the compiled tokens. Since that's only the docs I don't care too much and chose a different key...

@kulmann kulmann marked this pull request as draft August 18, 2022 15:01
Copy link
Member

@kulmann kulmann left a comment

Choose a reason for hiding this comment

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

Converted back to draft. Font embedding works now, but the oc-text-* classes feel broken. And still finetuning the font sizes...

@kulmann kulmann marked this pull request as ready for review August 19, 2022 08:32
Copy link
Member

@kulmann kulmann left a comment

Choose a reason for hiding this comment

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

good now 👍

@sonarcloud
Copy link

sonarcloud bot commented Aug 19, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@kulmann kulmann merged commit baf0d0b into master Aug 19, 2022
@delete-merged-branch delete-merged-branch bot deleted the make-ui-small-again branch August 19, 2022 17:04
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.

None yet

3 participants