Duplicate for Testing — GH-1975 Theme Framework, Leaf & Palm Themes #523
Conversation
…z feature buttons
Eden12345
changed the title
Eden12345
changed the title
| let protectedColor; | ||
| let allowedColor; | ||
| let restrictedColor; | ||
| switch (contextType) { | ||
| case 'palm-theme': | ||
| protectedColor = '#87f0fb'; | ||
| allowedColor = '#b8e986'; | ||
| restrictedColor = '#ff7e74'; | ||
| break; | ||
| case 'leaf-theme': | ||
| protectedColor = '#2e3b80'; | ||
| allowedColor = '#326c45'; | ||
| restrictedColor = '#963939'; | ||
| break; | ||
| default: | ||
| protectedColor = '#00AEF0'; | ||
| allowedColor = '#96c761'; | ||
| restrictedColor = '#00AEF0'; | ||
| break; | ||
| } | ||
|
|
wlycdgr
Apr 8, 2020
Member
I like inlining the SVGs, as per my other comment, but would it be better to set these attributes through CSS, so that there is a CSS file that is the single source of truth for all the styling values for any given theme? This does have the advantage of being a bit simpler and more direct than the CSS way, so I'm not sure. @Eden12345 - thoughts?
I like inlining the SVGs, as per my other comment, but would it be better to set these attributes through CSS, so that there is a CSS file that is the single source of truth for all the styling values for any given theme? This does have the advantage of being a bit simpler and more direct than the CSS way, so I'm not sure. @Eden12345 - thoughts?
Eden12345
Apr 8, 2020
Contributor
I think the CSS files should be the single source of truth, as you're saying. Since we're already using inline SVGs, it won't be too difficult to move these colors to a CSS file.
I think the CSS files should be the single source of truth, as you're saying. Since we're already using inline SVGs, it won't be too difficult to move these colors to a CSS file.
benstrumeyer
Apr 10, 2020
Author
Contributor
The CSS is now the single source of truth!
The CSS is now the single source of truth!
…te path for background images for palm and leaf theme
…Cliqz feature buttons
|
Looks good to me. Won't give it the ol' GitHub approval as we aren't merging this into |
| // const caretClasses = (this.state.isExpanded ? 'caret-up' : 'caret-down') + (isUnknown ? ' Category__antiTrackingCaret' : ''); | ||
| const caretClasses = ClassNames(this.context, { |
wlycdgr
Apr 13, 2020
Member
Did you mean to leave this?
Did you mean to leave this?
benstrumeyer
Apr 13, 2020
Author
Contributor
Ah, I did not. Deleted!
Ah, I did not. Deleted!
| <path d="M1.283 7L0 5.676 5.5 0 11 5.676 9.717 7 5.5 2.649z" fillRule="evenodd" /> | ||
| </svg> | ||
| ); | ||
| // 4a4a4a |
wlycdgr
Apr 13, 2020
Member
Can be deleted?
Can be deleted?
benstrumeyer
Apr 13, 2020
Author
Contributor
yes it can!
yes it can!
| // const caretClasses = (this.state.isExpanded ? 'caret-up' : 'caret-down') + (isUnknown ? ' Category__antiTrackingCaret' : ''); | ||
| // const caretClasses = (this.state.isExpanded ? this._renderCaret('up') : this._renderCaret('down')); |
wlycdgr
Apr 13, 2020
Member
Can be deleted?
Can be deleted?
benstrumeyer
Apr 13, 2020
Author
Contributor
yes it can!
yes it can!
| _renderCliqzCookiesAndFingerprintsIcon() { | ||
| return ( | ||
| <svg className="trk-cliqz-stats-icon" width="15" height="10" viewBox="0 0 10 10" xmlns="http://www.w3.org/2000/svg"> | ||
| <path className={`cookies-and-fingerprints-icon ${this.context}`} fill="#FFF" fillRule="evenodd" strokeWidth=".96" d="M5.085 1.013a.288.288 0 0 0-.17 0l-3.66.97A.328.328 0 0 0 1 2.308c.017 2.606 1.413 5.024 3.813 6.642.05.034.119.051.187.051a.344.344 0 0 0 .187-.051C7.587 7.33 8.983 4.913 9 2.307a.328.328 0 0 0-.255-.323l-3.66-.971z" /> | ||
| </svg> | ||
| ); | ||
| } |
wlycdgr
Apr 13, 2020
Member
Let's lift the class that identifies what sub-type of Cliqz icon this is up to the root svg element, since it applies to the whole thing. Let's also remove the fill attribute from the path element here to minimize confusion, since it's specified per-theme in the CSS.
Let's lift the class that identifies what sub-type of Cliqz icon this is up to the root svg element, since it applies to the whole thing. Let's also remove the fill attribute from the path element here to minimize confusion, since it's specified per-theme in the CSS.
benstrumeyer
Apr 13, 2020
Author
Contributor
Love this idea--Moved the class up to the selector, removed the fill attribute in the JSX and made the CSS the single source of truth!
Love this idea--Moved the class up to the selector, removed the fill attribute in the JSX and made the CSS the single source of truth!
| <svg className="trk-cliqz-stats-icon" width="15" height="10" viewBox="0 0 11 11" xmlns="http://www.w3.org/2000/svg"> | ||
| <g fill="none" fillRule="evenodd"> | ||
| <path fill="#FFF" d="M7.49 1.234L9.922 3.89l-.157 3.6L7.11 9.922l-3.6-.157L1.078 7.11l.157-3.6L3.89 1.078z" /> | ||
| <path className={`ads-icon ${this.context}`} stroke="1DAFED" d="M2.788 8.54c.315.315.628.63.944.943.023.023.067.035.103.035 1.077.001 2.153.002 3.23-.001.04 0 .09-.02.117-.048a820.63 820.63 0 0 0 2.285-2.285.184.184 0 0 0 .05-.116c.003-1.08.003-2.16.002-3.24-.001-.03-.008-.068-.026-.088-.316-.321-.635-.64-.95-.956L2.789 8.54m-.436-.433l5.754-5.754c-.308-.309-.621-.623-.937-.936a.16.16 0 0 0-.102-.036 709.213 709.213 0 0 0-3.231 0c-.04 0-.09.02-.118.048-.765.762-1.53 1.525-2.291 2.29a.16.16 0 0 0-.045.1 928.271 928.271 0 0 0 0 3.26c0 .029.01.065.03.085.314.318.631.634.94.943m7.752-2.652c0 .581-.002 1.162.002 1.743a.405.405 0 0 1-.127.31 879.44 879.44 0 0 0-2.47 2.47.398.398 0 0 1-.303.128c-1.17-.003-2.341-.003-3.512 0a.4.4 0 0 1-.302-.126A884.3 884.3 0 0 0 .915 7.503a.385.385 0 0 1-.121-.294c.002-1.17.002-2.342 0-3.513 0-.122.036-.216.123-.303.827-.824 1.653-1.65 2.477-2.477a.388.388 0 0 1 .293-.123c1.174.002 2.348.002 3.523 0 .119 0 .21.038.293.122.827.83 1.655 1.657 2.484 2.484.081.08.12.17.119.285-.004.59-.002 1.181-.002 1.771" /> | ||
| </g> | ||
| </svg> |
wlycdgr
Apr 13, 2020
Member
Same deal with the subtype class and fill and stroke attributes here - let's lift up the former and remove the latter. Also, since (I think?) the fill and/or stroke attributes of both paths can vary depending on the fill, let's make sure that we can modify both from the CSS. Could use selectors, could give the first path a class too.
Same deal with the subtype class and fill and stroke attributes here - let's lift up the former and remove the latter. Also, since (I think?) the fill and/or stroke attributes of both paths can vary depending on the fill, let's make sure that we can modify both from the CSS. Could use selectors, could give the first path a class too.
benstrumeyer
Apr 13, 2020
Author
Contributor
Lifted the subtype classes, and made sure we can change the fill/stroke values inside the CSS. Love the idea of using a class name for the first path since it's nice to see which fill we're changing.
Lifted the subtype classes, and made sure we can change the fill/stroke values inside the CSS. Love the idea of using a class name for the first path since it's nice to see which fill we're changing.
| const borderClassNames = ClassNames('border', { | ||
| protected: type === 'antiTracking', | ||
| restricted: type !== 'antiTracking', | ||
| 'palm-theme': contextType === 'palm-theme', | ||
| 'leaf-theme': contextType === 'leaf-theme' | ||
| }); | ||
|
|
||
| const backgroundClassNames = ClassNames('background', { | ||
| protected: type === 'antiTracking', | ||
| restricted: type !== 'antiTracking', | ||
| 'palm-theme': contextType === 'palm-theme', | ||
| 'leaf-theme': contextType === 'leaf-theme' | ||
| }); | ||
|
|
wlycdgr
Apr 13, 2020
Member
I could be tripping but I think you can just do restricted: type !== 'antiTracking', contextType,, right? Or will having an extraneous default/dark-theme class sometimes mess something up?
I could be tripping but I think you can just do restricted: type !== 'antiTracking', contextType,, right? Or will having an extraneous default/dark-theme class sometimes mess something up?
benstrumeyer
Apr 13, 2020
Author
Contributor
You are def not tripping, I didn't know I could just slap a variable in there--thanks for the trick. Also we don't have any default / dark-theme specific CSS for these buttons so this works out!
You are def not tripping, I didn't know I could just slap a variable in there--thanks for the trick. Also we don't have any default / dark-theme specific CSS for these buttons so this works out!
| <path className={borderClassNames} fill="#FFF" d="M-.5-.5h18.3v18.217H-.5z" /> | ||
| <path className={backgroundClassNames} stroke="#FFF" d="M.5.5h16.3v16.217H.5z" /> |
wlycdgr
Apr 13, 2020
Member
Like with the Cliqz icons, since the fill and stroke are theme-dependent, let's define them entirely in the CSS
Like with the Cliqz icons, since the fill and stroke are theme-dependent, let's define them entirely in the CSS
benstrumeyer
Apr 13, 2020
Author
Contributor
Moved fill and stroke to the selectors in the CSS!
Moved fill and stroke to the selectors in the CSS!
| function getThemeColor() { | ||
| switch (StatsGraph.context) { | ||
| case 'palm-theme': | ||
| return '#172a0b'; | ||
| case 'leaf-theme': | ||
| return '#173700'; | ||
| default: | ||
| return '#124559'; | ||
| } | ||
| } | ||
|
|
wlycdgr
Apr 13, 2020
Member
There's not a different color for dark-theme? Or were you planning to add that after this PR
There's not a different color for dark-theme? Or were you planning to add that after this PR
benstrumeyer
Apr 13, 2020
Author
Contributor
Hmm, it looks like #124559 is exact blue we use for the midnight theme and we use the same blue across the default as well
Thoughts?
Hmm, it looks like #124559 is exact blue we use for the midnight theme and we use the same blue across the default as well
Thoughts?
| const { user, loggedIn } = this.props; | ||
| const { showResetModal, selection } = this.state; | ||
| return ( | ||
| <StatsView | ||
| showResetModal={this.state.showResetModal} | ||
| showPitchModal={!this.props.user || !this.props.user.subscriptionsPlus} | ||
| loggedIn={this.props.loggedIn} | ||
| showResetModal={showResetModal} | ||
| showPitchModal={!user || !user.subscriptionsPlus} | ||
| loggedIn={loggedIn} | ||
| getStats={this.getStats} | ||
| selection={this.state.selection} | ||
| selection={selection} |
wlycdgr
Apr 13, 2020
Member
Niiice
Niiice
benstrumeyer
Apr 13, 2020
Author
Contributor
thank you
thank you
| @@ -22,7 +22,7 @@ import Stats from '../components/Stats'; | |||
| * @todo We are not using ownProps, so we better not specify it explicitly, | |||
| * in this case it won't be passed by React (see https://github.com/reactjs/react-redux/blob/master/docs/api.md). | |||
| */ | |||
| const mapStateToProps = state => Object.assign({}, state.account); | |||
| const mapStateToProps = state => Object.assign({}, state.account, {}); | |||
wlycdgr
Apr 13, 2020
Member
Is that empty object at the end intended?
Is that empty object at the end intended?
benstrumeyer
Apr 13, 2020
Author
Contributor
Just a remnant of when I was adding theme props to this container before we changed to context. Deleted!
Just a remnant of when I was adding theme props to this container before we changed to context. Deleted!
…tes in jsx for ads and cookies & fingerprints icon
feature/palm-themeand we can merge that one into develop