Style icons with CSS #2272

jimbo · 2020-03-19T22:50:36Z

Description

Style the Icon component with CSS rather than SVG attributes.

Related Issue

PWA-445

Acceptance

@tjwiebell

Verification Stakeholders

Specification

Verification Steps

View icons throughout the site, verifying none have changed.
Fill and empty your cart, verifying that the icon changes when you do.

Screenshots / Screen Captures (if appropriate)

Checklist

I have updated the documentation accordingly, if necessary.
I have added tests to cover my changes, if necessary.

PWAStudioBot · 2020-03-19T23:00:22Z

	Messages
📖	Access a deployed version of this PR here. Make sure to wait for the "pwa-pull-request-deploy" job to complete.
📖	DangerCI Failures related to missing labels/description/linked issues/etc will persist until the next push or next nightly build run (assuming they are fixed).
📖	Associated JIRA tickets: PWA-445.

Generated by 🚫 dangerJS against f3168e1

sirugh · 2020-03-20T15:18:39Z

packages/venia-ui/lib/components/Header/cartTrigger.css

+    composes: root from '../Icon/icon.css';
+}
+
+.icon--filled {


So instead of passing a color prop, if you want to override the filled color you would provide your own icon--filled class, right?

sirugh

A few comments regarding propTypes but otherwise this looks good. Shouldn't be anything major that would hold this up though.

sirugh · 2020-03-25T19:45:44Z

packages/venia-ui/lib/components/Header/cartTrigger.js

 CartTrigger.propTypes = {
-    iconColor: string,
    classes: shape({
        root: string


Can you add any missing classes? This is our only documentation of the change.

sirugh · 2020-03-25T19:51:31Z

packages/venia-ui/lib/components/Icon/icon.js

+    attrs: shape({}),
+    classes: shape({
+        'icon--empty': string,
+        'icon--filled': string,


I don't think this is accurate - we aren't passing icon--empty or icon--filled with classes. We apply one or the other to the Icon itself via className. It just happens to get spread onto the Icon through restProps.

Oh, I see what I did. I put the new classnames in the prop types for Icon rather than CartTrigger. I'll fix that.

As for className as a prop, you're right. The classify HOC allowed us to pass className as a shortcut override for classes.root, but its mergeClasses utility is simpler and doesn't have that feature. I'll update CartTrigger to override classes.root more explicitly.

dpatil-magento · 2020-03-26T21:43:11Z

@jimbo Pls take a look on following observations , PR (left) vs develop (Right)
1 No green border in Kebab icons, qty steppers ui.
2. No X icon, right mark in price check box and color .
3. Pagination icons differ.
4. Sort select icon, right mark missing.

jimbo · 2020-03-30T23:32:12Z

No green border in Kebab icons, qty steppers ui

Fixed both. Kebab icons are now slightly larger; stepper icons are now slightly smaller.

No X icon, right mark in price check box and color

Fixed both. The synthetic checkboxes now use raw feather icons with a new appearance.

Pagination icons differ.

Prefer the new unfilled style, so this is fine.

Sort select icon, right mark missing.

Fixed. Checkmark now uses raw feather icons with a new appearance.

dpatil-magento · 2020-03-31T15:33:13Z

@jimbo Fixes looks good but problem with paginations icons issue is, they don't get disabled when user is first or last page.

jimbo · 2020-03-31T17:56:34Z

Fixes looks good but problem with paginations icons issue is, they don't get disabled when user is first or last page.

Good catch. Fixed. Also fixed the style of those buttons to be properly square and a bit larger (32x32).

dpatil-magento · 2020-03-31T18:36:00Z

UI looks good on Chrome, Safari, firefox, android chrome. Good to merge once latest commits are reviewed.

tjwiebell

Just need clarification on a redundant class, otherwise these changes look good 👍

tjwiebell · 2020-03-31T19:20:23Z

packages/venia-ui/lib/components/Pagination/navButton.css

-    composes: buttonArrow;
-    color: #999;
+.root_disabled {
+    composes: root;


This class seems a bit redundant. Did you intend to add additional styles, or should this instead just be:

.root:disabled { pointer-events: none; }

Fair point. I had included it as an optional classname to mirror the one for the icon, but the pseudo is probably sufficient for the root itself.

While I was fixing this, I noticed a few more bits of weird CSS in the pagination styles. These have been fixed. Please check those out too. @tjwiebell

jimbo added 2 commits March 19, 2020 15:36

Refactor icon style

4781520

Update snapshots

c373ede

jimbo added the version: Major This changeset includes incompatible API changes and its release necessitates a Major version bump. label Mar 19, 2020

PWAStudioBot added the pkg:venia-ui label Mar 19, 2020

sirugh reviewed Mar 20, 2020

View reviewed changes

m2-community-project bot assigned sirugh Mar 20, 2020

m2-community-project bot added the Progress: review label Mar 20, 2020

sirugh requested changes Mar 25, 2020

View reviewed changes

Fix icon classnames and prop types

ab8638e

m2-community-project bot assigned jimbo Mar 26, 2020

sirugh previously approved these changes Mar 26, 2020

View reviewed changes

Merge branch 'develop' into jimbo/icon-style

afcfecc

Update quantity stepper and kebab icons

9dce59f

jimbo dismissed sirugh’s stale review via 9dce59f March 30, 2020 22:11

Fix filter and sort icons

ee808a1

Merge branch 'develop' into jimbo/icon-style

2bc53d4

Fix disabled pagination buttons

b6c13eb

Merge branch 'develop' into jimbo/icon-style

3756821

tjwiebell suggested changes Mar 31, 2020

View reviewed changes

m2-community-project bot assigned tjwiebell Mar 31, 2020

m2-community-project bot removed the Progress: review label Mar 31, 2020

m2-community-project bot added the Progress: changes requested label Mar 31, 2020

Fix pagination css

f3168e1

tjwiebell approved these changes Apr 1, 2020

View reviewed changes

dpatil-magento merged commit 25e8d20 into develop Apr 1, 2020

dpatil-magento deleted the jimbo/icon-style branch April 1, 2020 15:04

m2-community-project bot removed the Progress: changes requested label Apr 1, 2020

awilcoxa mentioned this pull request Apr 9, 2020

[bug]: Pagination div active it's outside #2296

Closed

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Style icons with CSS #2272

Style icons with CSS #2272

jimbo commented Mar 19, 2020

PWAStudioBot commented Mar 19, 2020 •

edited

Loading

sirugh Mar 20, 2020

sirugh left a comment

sirugh Mar 25, 2020

sirugh Mar 25, 2020

jimbo Mar 26, 2020

dpatil-magento commented Mar 26, 2020 •

edited

Loading

jimbo commented Mar 30, 2020

dpatil-magento commented Mar 31, 2020

jimbo commented Mar 31, 2020 •

edited

Loading

dpatil-magento commented Mar 31, 2020

tjwiebell left a comment

tjwiebell Mar 31, 2020 •

edited

Loading

jimbo Mar 31, 2020

jimbo Mar 31, 2020

Style icons with CSS #2272

Style icons with CSS #2272

Conversation

jimbo commented Mar 19, 2020

Description

Related Issue

Acceptance

Verification Stakeholders

Specification

Verification Steps

Screenshots / Screen Captures (if appropriate)

Checklist

PWAStudioBot commented Mar 19, 2020 • edited Loading

sirugh Mar 20, 2020

Choose a reason for hiding this comment

sirugh left a comment

Choose a reason for hiding this comment

sirugh Mar 25, 2020

Choose a reason for hiding this comment

sirugh Mar 25, 2020

Choose a reason for hiding this comment

jimbo Mar 26, 2020

Choose a reason for hiding this comment

dpatil-magento commented Mar 26, 2020 • edited Loading

jimbo commented Mar 30, 2020

dpatil-magento commented Mar 31, 2020

jimbo commented Mar 31, 2020 • edited Loading

dpatil-magento commented Mar 31, 2020

tjwiebell left a comment

Choose a reason for hiding this comment

tjwiebell Mar 31, 2020 • edited Loading

Choose a reason for hiding this comment

jimbo Mar 31, 2020

Choose a reason for hiding this comment

jimbo Mar 31, 2020

Choose a reason for hiding this comment

PWAStudioBot commented Mar 19, 2020 •

edited

Loading

dpatil-magento commented Mar 26, 2020 •

edited

Loading

jimbo commented Mar 31, 2020 •

edited

Loading

tjwiebell Mar 31, 2020 •

edited

Loading