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

Fix design issues on social buttons #2073

Draft
wants to merge 17 commits into
base: main
Choose a base branch
from
Draft

Conversation

hannahiss
Copy link
Member

@hannahiss hannahiss commented Jun 8, 2023

Note: Please transform - [ ] into - (NA) in the description when things are not applicable

Related issues

Issues seen when reviewing issue #1766 and PR #1778

Description

What has been seen by Franco:

  • global quality of social icons is not really good and some icons are not compliant with real ones
  • design issues:
    • hover color of Snapchat should be black and not white
    • Facebook icon is not correct
    • LinkedIn icon size should be 18x18 instead of 20x20

Franco proposed new icons, they have been optimized with svgo and cleaned up of unnecessary attributes, and result is obviously visually better but bundle size is really bigger. Note that some aliasing is still visible in zoom 100% but seems due to browser display (ok in 200%).

Icons look and size comparison, and proposition:

icon old size new size Look enhancement proposition
Facebook 141 bytes 257 bytes really better use new icon
Instagram 840 bytes 1 664 bytes slightly better (visible only with zoom 200%) keep old icon
WhatsApp 511 bytes 1 216 bytes slightly better (visible only with zoom 200%) keep old icon
LinkedIn 239 bytes 413 bytes really better use new icon
Youtube 414 bytes 411 bytes slightly better (visible only with zoom 200%) use new icon
Snapchat 206 bytes 1 532 bytes really better find other icon?
Pinterest 292 bytes 832 bytes a bit better and bigger keep old icon?
Email 502 bytes 380 bytes a bit better and bigger use new icon
Total 122 748 bytes 126 353 bytes

Comparison before (above) and after (below):
image

No changes on icons Twitter (for now) and TikTok (already new).

Technical changes in .btn-social class:

  • transform can now be specified on all icons if needed, not only LinkedIn (to make code more generic)
  • new possibility to change hover color if specified in $btn-social-networks in _variables.scss

Motivation & Context

Changes asked by Franco after a review on social icons.

Types of change

  • Bug fix (non-breaking which fixes an issue)

Live previews

Checklist

Contribution

Accessibility

  • My change follows accessibility good practices; I have at least run axe

Design

  • My change respects the design guidelines defined in Orange Design System
  • (NA) My change is compatible with responsive display

Development

  • My change follows the developer guide
  • (NA) I have added JavaScript unit tests to cover my changes
  • (NA) I have added SCSS unit tests to cover my changes

Documentation

  • My change introduces changes to the documentation and/or I have updated the documentation accordingly

Checklist (for Core Team only)

  • (NA) My change introduces changes to the migration guide
  • (NA) My new component is added in Storybook
  • (NA) My new component is compatible with RTL
  • (NA) Manually run BrowserStack tests
  • (NA) Manually test browser compatibility with BrowserStack (Chrome >= 60, Firefox >= 60 (+ ESR), Edge, Safari >= 12, iOS Safari, Chrome & Firefox on Android)
  • Code review
  • Design review
  • A11y review

After the merge

- add Tiktok icon
- change svg since their quality wasn't good enough, optimize them with svgo
- change hover color for Snapchat to black
- adjust sizes, change LinkedIn size to 18x18 instead of 20x20 to conform to design
# Conflicts:
#	site/content/docs/5.2/migration.md
# Conflicts:
#	site/content/docs/5.3/migration.md
@netlify
Copy link

netlify bot commented Jun 8, 2023

Deploy Preview for boosted ready!

Name Link
🔨 Latest commit 43784d6
🔍 Latest deploy log https://app.netlify.com/sites/boosted/deploys/6596d5eccd828600096f6b90
😎 Deploy Preview https://deploy-preview-2073--boosted.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@sonarcloud
Copy link

sonarcloud bot commented Jun 8, 2023

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

# Conflicts:
#	scss/_variables.scss
@hannahiss hannahiss marked this pull request as ready for review August 24, 2023 12:26
@hannahiss hannahiss marked this pull request as draft August 24, 2023 12:29
@hannahiss
Copy link
Member Author

hannahiss commented Aug 29, 2023

During the web weekly synch on 29th August, we agreed on putting this PR "on hold" and start a technical study about using external svgs instead of embedded ones in our css file. We have to check how to manage other styles like transform, size and hiver color (class, css variable?)

@julien-deramond
Copy link
Member

julien-deramond commented Aug 29, 2023

Quick prototype can be done with the following idea:

<a href="#" class="btn btn-icon btn-social" style="--bs-network-color: #3b5998;
--bs-network-logo: url(&quot;data:image/svg+xml,%3csvg xmlns='http://www.w3.org/2000/svg' viewBox='0 0 32 32'%3e%3cpath d='M19 6h5V0h-5c-4 0-7 3-7 7v3H8v6h4v16h6V16h5l1-6h-6V7l1-1z'%3e%3c/path%3e%3c/svg%3e&quot;);">
  <span class="visually-hidden">Facebook</span>
</a>
diff --git a/scss/_buttons.scss b/scss/_buttons.scss
index 7e08d0d2d..d0590f11b 100644
--- a/scss/_buttons.scss
+++ b/scss/_buttons.scss
@@ -248,6 +248,10 @@
     --#{$prefix}btn-active-border-color: #{$white};
     --#{$prefix}btn-disabled-color: #{$gray-700};
   }
+
+  &::before {
+    mask-size: 32; /* It can be a CSS var as well if needed */
+  }
 }
 
 @each $name in map-keys($btn-social-networks) {

Then the doc could present the different icons and CSS variables to use as inline CSS, or explain that projects can still create their .btn-network and just define the 2 CSS vars within this class definition.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: In Progress / Draft
Development

Successfully merging this pull request may close these issues.

3 participants