solidusio / solidus_starter_frontend

🎨 Rails-based starter kit for your Solidus storefront.
BSD 3-Clause "New" or "Revised" License
58 stars 40 forks source link

Enhancement: Remove unused fonts and replace remaining fonts with similar open lic… #380

Open fthobe opened 3 days ago

fthobe commented 3 days ago

Summary

This PR brings following improvements:

Following files have been modified as part of this commit:

Fixes: #379 #381

Checklist

Check out our PR guidelines for more details.

The following are mandatory for all PRs:

The following are not always needed:

codecov[bot] commented 2 days ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 95.96%. Comparing base (bb0ed35) to head (e28df85).

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #380 +/- ## ========================================== - Coverage 95.98% 95.96% -0.02% ========================================== Files 207 207 Lines 4878 4857 -21 ========================================== - Hits 4682 4661 -21 Misses 196 196 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.


🚨 Try these New Features:

jarednorman commented 2 days ago

This is a visual change as changes the fonts, but is missing screenshots showing the before/after.

fthobe commented 2 days ago

The replacement of the font was a compliance issue, not a style contest. Please reach out to @kennyadsl about this. We will have to delete and hard fork our front-end repo if they are not removed. If you prefer a different font feel free to suggest it, just know that we can't provide any contributions till this is resolved.

kennyadsl commented 2 days ago

Yes, we need to replace that font. As Fabian kindly pointed out, we cannot use that font on an open-source project (we bought the license for solidus.io. and its subdomains but apparently its usage here is not allowed). It needs to be replaced asap.

@fthobe true, this is not driven by an aesthetic requirement but has a visual impact, so if you have some screenshots of the starter frontend with the new font, it might speed up the merge. Let me know if you need any help with them.

kennyadsl commented 2 days ago

BTW, we have some screenshot as artifact on the CircleCI failures:

failures_r_spec_example_groups_searching_products_clicks_on_a_suggestion_pressing_enter_147 failures_r_spec_example_groups_searching_products_allows_mouse_click_on_results_339

fthobe commented 2 days ago

BTW, we have some screenshot as artifact on the CircleCI failures:

@jarednorman Does that suffice?

jarednorman commented 1 day ago

Yes, this is good. While the change wasn't motivated by style, I still wanted to see how it changed to make sure we were choosing a reasonable alternate. Works for me.

fthobe commented 13 hours ago

Yes, this is good. While the change wasn't motivated by style, I still wanted to see how it changed to make sure we were choosing a reasonable alternate. Works for me.

Can we merge this please, I can't run anything with licensing problems on our systems.