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

GH-1975 Theme Framework & GH-1972 Palm Theme #517

Closed
wants to merge 22 commits into from
Closed
Changes from all commits
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
File filter
Filter file types
Jump to
Jump to file
Failed to load files.

Always

Just for now

@@ -0,0 +1,7 @@
1. Two theme scss files palm-theme.scss and midnight-theme.scss are added under app\scss
2. Couple of lines in .\webpack.config.js ensure the themes are compiled as the rest of SCSS files.
They end up in dist\css as palm_theme.css and midnight_theme.css. Will refer to them by file paths.
3. In src\classes\account.js getTheme function mimics the original, but returns file paths instead of
css stream.
4. In app\panel\utils\utils.js setTheme function is changed to add <link rel="stylesheet" href={path_to_theme}/>
element instead of inline <style> element.
@@ -1817,8 +1817,17 @@
"subscribe_pitch_sign_in": {
"message": "Already subscribed? Sign in"
},
"subscription_midnight_theme": {
"message": "Midnight Theme"
"subscription_default_theme": {
"message": "Default"
},
"subscription_dark_blue_theme": {
"message": "Dark Blue Theme"
},
"subscription_palm_theme": {
"message": "Palm Theme"
},
"subscription_leaf_theme": {
"message": "Leaf Theme"
},
"subscription_status": {
"message": "Status"
@@ -0,0 +1,6 @@
<svg width="16" height="16" viewBox="0 0 16 16" xmlns="http://www.w3.org/2000/svg">
<g transform="translate(1)" fill="none" fill-rule="evenodd">
<circle stroke="#2092bf" cx="7" cy="8" r="7"/>
<path d="M5.86 4.756c0-.582.326-.873.974-.873.648 0 .973.29.973.873 0 .277-.08.493-.244.647-.162.155-.405.232-.73.232-.647 0-.972-.293-.972-.88zM7.726 13H5.938V6.45h1.787V13z" fill="#2092bf"/>
</g>
</svg>
Binary file not shown.

Large diffs are not rendered by default.

@@ -0,0 +1,7 @@
<?xml version="1.0" encoding="utf-8"?>
<svg width="10" height="10" viewBox="0 0 11 11" xmlns="http://www.w3.org/2000/svg">
<g fill="none" fill-rule="evenodd">
<path fill="#172a0b" 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 fill="#87f0fb" stroke="#87f0fb" stroke-width=".5" 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>
@@ -0,0 +1,4 @@
<?xml version="1.0" encoding="utf-8"?>
<svg width="10" height="10" viewBox="0 0 10 10" xmlns="http://www.w3.org/2000/svg">
<path fill="#172a0b" fill-rule="evenodd" stroke="#87f0fb" stroke-width=".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>
@@ -294,7 +294,8 @@ class Blocking extends React.Component {
show_tracker_urls,
sitePolicy,
smartBlock,
smartBlockActive
smartBlockActive,
current_theme,
} = this.props;

const {
@@ -330,6 +331,7 @@ class Blocking extends React.Component {
smartBlock={smartBlock}
unknownCategory={unknownCategory}
enable_anti_tracking={enable_anti_tracking}
current_theme={current_theme}

This comment has been minimized.

@wlycdgr

wlycdgr Mar 27, 2020
Member

This is a good use case for contexts, which allow you to make this kind of 'global' setting available to every component in the tree without manually passing the value down component by component. In fact, themes are the example used in the context React docs! - https://reactjs.org/docs/context.html

This comment has been minimized.

@benstrumeyer

benstrumeyer Apr 6, 2020
Author Contributor

Used context for the first time! It's soo clean

/>
)}
</div>
@@ -35,6 +35,7 @@ class Categories extends React.Component {
unknownCategory,
enable_anti_tracking,
sitePolicy,
current_theme,
} = this.props;
const globalBlocking = !!this.props.globalBlocking;
const filtered = !!this.props.filtered;
@@ -90,6 +91,7 @@ class Categories extends React.Component {
smartBlock={this.props.smartBlock}
enable_anti_tracking={enable_anti_tracking}
isUnknown={isUnknown}
current_theme={current_theme}
/>
);
};
@@ -164,6 +164,7 @@ class Category extends React.Component {
paused_blocking,
sitePolicy,
isUnknown,
current_theme,
} = this.props;

const globalBlocking = !!this.props.globalBlocking;
@@ -270,6 +271,7 @@ class Category extends React.Component {
smartBlockActive={this.props.smartBlockActive}
smartBlock={this.props.smartBlock}
isUnknown={isUnknown}
current_theme={current_theme}
/>
)}
</div>
@@ -259,7 +259,8 @@ class Tracker extends React.Component {
_renderCliqzAdsIcon() { return this._renderCliqzStatsIcon('ads'); }

_renderCliqzStatsIcon(type) {
const path = `/app/images/panel/tracker-detail-cliqz-${type}-icon.svg`;
const { current_theme } = this.props;
const path = `/app/images/panel/tracker-detail-cliqz-${type}-${current_theme}-icon.svg`;

return (
<ReactSVG src={path} className="trk-cliqz-stats-icon" />
@@ -40,7 +40,7 @@ class Trackers extends React.Component {
* @return {ReactComponent} ReactComponent instance
*/
render() {
const { trackers, isUnknown } = this.props;
const { trackers, isUnknown, current_theme } = this.props;
let trackerList;
if (this.props.globalBlocking) {
const trackersToShow = [];
@@ -75,6 +75,7 @@ class Trackers extends React.Component {
smartBlockActive={this.props.smartBlockActive}
smartBlock={this.props.smartBlock}
isUnknown={isUnknown}
current_theme={current_theme}
/>
));
}
@@ -51,7 +51,13 @@ class CliqzFeature extends React.Component {
}

_getStatus(active) {
return active ? t('on') : t('off');
const { current_theme, trackerCount } = this.props;
switch (current_theme) {
case 'palm-theme':
return active ? (<span>{trackerCount}</span>) : (<div className="rectangle" />);
default:
return active ? t('on') : t('off');
}
}

_getTooltipBodyText(active, isTooltipBody, type) {
@@ -0,0 +1,46 @@
/**
* Radio Button Component
*
* Ghostery Browser Extension
* https://www.ghostery.com/
*
* Copyright 2019 Ghostery, Inc. All rights reserved.
*
* This Source Code Form is subject to the terms of the Mozilla Public
* License, v. 2.0. If a copy of the MPL was not distributed with this
* file, You can obtain one at http://mozilla.org/MPL/2.0
*/

/* eslint jsx-a11y/label-has-associated-control: 0 */

This comment has been minimized.

This comment has been minimized.

@benstrumeyer

benstrumeyer Mar 31, 2020
Author Contributor

Definitely will try to resolve those before resorting to eslint, I think I mistakenly put that here since there are no labels


import React from 'react';
import PropTypes from 'prop-types';
import ClassNames from 'classnames';

/**
* @class Implements a single radio button to be used inside the RadioButtonGroup component
* @memberof PanelBuildingBlocks
*/

const RadioButton = (props) => {

This comment has been minimized.

@wlycdgr

wlycdgr Mar 27, 2020
Member

Destructure to clean up a lil
const { checked, handleClick } = props;, etc

This comment has been minimized.

@benstrumeyer

benstrumeyer Mar 31, 2020
Author Contributor

Destructured!

const OuterCircleClassNames = ClassNames('RadioButton__outerCircle', {
checked: props.checked,
});
const InnerCircleClassNames = ClassNames('RadioButton__innerCircle', {
checked: props.checked,
});
return (
<span>
<span className={OuterCircleClassNames} onClick={props.handleClick}>
<span className={InnerCircleClassNames} />
</span>
</span>
);
};

// PropTypes ensure we pass required props of the correct type
RadioButton.propTypes = {
handleClick: PropTypes.func.isRequired,

This comment has been minimized.

@wlycdgr

wlycdgr Mar 27, 2020
Member

add checked

This comment has been minimized.

@benstrumeyer

benstrumeyer Mar 31, 2020
Author Contributor

Added checked!

};

export default RadioButton;
@@ -0,0 +1,74 @@
/**
* Radio Button Group Component
*
* Ghostery Browser Extension
* https://www.ghostery.com/
*
* Copyright 2019 Ghostery, Inc. All rights reserved.
*
* This Source Code Form is subject to the terms of the Mozilla Public
* License, v. 2.0. If a copy of the MPL was not distributed with this
* file, You can obtain one at http://mozilla.org/MPL/2.0
*/

/* eslint jsx-a11y/label-has-associated-control: 0 */

import React from 'react';
import PropTypes from 'prop-types';
import RadioButton from './RadioButton';

/**
* @class Implements a radio button group
* @memberof PanelBuildingBlocks
*/
class RadioButtonGroup extends React.Component {
constructor(props) {
super(props);
this.state = {
buttons: []

This comment has been minimized.

@wlycdgr

wlycdgr Mar 27, 2020
Member

buttons suggests that the items are button objects/components. Use a name that better reflects the nature of the (on/off bool) elements.

This comment has been minimized.

@benstrumeyer

benstrumeyer Mar 31, 2020
Author Contributor

Changed to buttonState! Actually, removed altogether per Ethan's comment:

It looks like this is the only place that you're using the local state in this component. You can instead just pass the props to the <RadioButton /> component like this:

const { indexClicked, handleItemClick } = this.props
// ...code in between...
	<RadioButton
		checked={index === indexClicked}
		handleClick={() => handleItemClick(index)}
	/>

Then, you can remove all of the code from componentDidMount and RadioButtonGroup.handleClick and not have to worry about local state.

};
}

componentDidMount() {
const buttons = new Array(this.props.items.length).fill(false);
buttons[this.props.selectedIndex] = true;
this.setState({ buttons });
}

handleClick(indexClicked) {
const { buttons } = this.state;
const updatedButtons = buttons.map((button, index) => (index === indexClicked));
this.setState({ buttons: updatedButtons });
this.props.handleItemClick(indexClicked);
}

/**
* React's required render function. Returns JSX
* @return {JSX} JSX for rendering the Toggle Slider used throughout the extension
*/
render() {
const { buttons } = this.state;
return (
this.props.items.map((item, index) => (
<div className="flex-container align-justify RadioButtonGroup__container" key={`${index * 2}`}>

This comment has been minimized.

@wlycdgr

wlycdgr Mar 27, 2020
Member

React docs advise against using array indices as keys if there's another unique identifier available. Can we use the labels?

This comment has been minimized.

@benstrumeyer

benstrumeyer Mar 31, 2020
Author Contributor

Yes we can!

<span className="RadioButtonGroup__label">
{t(item.text)}
</span>
<div>
<RadioButton checked={buttons[index]} handleClick={() => this.handleClick(index)} />
</div>
</div>
))
);
}
}

// PropTypes ensure we pass required props of the correct type
RadioButtonGroup.propTypes = {
items: PropTypes.arrayOf(PropTypes.object).isRequired, // Number of objects in array is the number of radio buttons

This comment has been minimized.

@wlycdgr

wlycdgr Mar 27, 2020
Member

Since we only use the text property of these objects for the individual button labels, let's pass in the labels only, as an array of strings. Not only do we avoid passing unused data that way but we also make the component more flexible, since it will no longer be expeting an object prop with a Themes-view-specific shape.

This comment has been minimized.

@benstrumeyer

benstrumeyer Mar 31, 2020
Author Contributor

So reusable. Passed in labels for flexibility

handleItemClick: PropTypes.func.isRequired,
selectedIndex: PropTypes.number.isRequired
};


export default RadioButtonGroup;
@@ -161,6 +161,15 @@ class StatsGraph extends React.Component {
.attrTween('stroke-dasharray', interpolator);
}

function getThemeColor(theme) {

This comment has been minimized.

@wlycdgr

wlycdgr Mar 27, 2020
Member

Nice

switch (theme) {
default:
return '#124559';
case 'palm-theme':
return '#172a0b';
}
Comment on lines +166 to +170

This comment has been minimized.

@fcjr

fcjr Mar 28, 2020
Member

Ignore my last comment, the early returns makes this work 😬I'm dumb, but regardless, can you swap the order of these statements, switches read a bit clearer when the default case is the last one

This comment has been minimized.

@benstrumeyer

benstrumeyer Mar 31, 2020
Author Contributor

Oh wow, I didn't know it worked like that. I'll always put default on the bottom then. Either way, good find

}

pathGroup.append('path')
.style('stroke', '#dddddd')
.style('stroke-dasharray', '4,4')
@@ -170,7 +179,7 @@ class StatsGraph extends React.Component {
pathGroup.append('path')
.attr('d', line)
.attr('fill', 'none')
.attr('stroke', '#124559')
.attr('stroke', getThemeColor(this.props.theme))
.attr('stroke-width', 1.5)
.call(animator);
// ---------------------------------------------------------------------- //
@@ -187,7 +196,6 @@ class StatsGraph extends React.Component {
.style('opacity', opacity);
}, 1);
}

// Add data points with event listeners for opening their respective tooltips
canvas.append('g')
.attr('class', 'point-group')
@@ -196,7 +204,7 @@ class StatsGraph extends React.Component {
.enter()
.append('circle')
.attr('class', (d, i) => `point point-${i}`)
.attr('fill', '#124559')
.attr('fill', getThemeColor(this.props.theme))
.attr('cx', d => x(d.index))
.attr('cy', d => y(d.amount))
.attr('r', 0)
@@ -21,6 +21,8 @@ import GhosteryFeature from './GhosteryFeature';
import NotScanned from './NotScanned';
import PauseButton from './PauseButton';
import ToggleSlider from './ToggleSlider';
import RadioButtonGroup from './RadioButtonGroup';
import RadioButton from './RadioButton';
import ModalExitButton from './ModalExitButton';

export {
@@ -31,5 +33,7 @@ export {
NotScanned,
PauseButton,
ToggleSlider,
RadioButtonGroup,
RadioButton,
ModalExitButton
};
@@ -524,11 +524,12 @@ class Stats extends React.Component {
* @return {ReactComponent} StatsView instance
*/
render() {
const { user, loggedIn, current_theme } = this.props;

This comment has been minimized.

This comment has been minimized.

@wlycdgr

wlycdgr Mar 27, 2020
Member

Let's do same for this.state while we're at it.

This comment has been minimized.

@benstrumeyer

benstrumeyer Mar 31, 2020
Author Contributor

Destructured this.state as well

return (
<StatsView
showResetModal={this.state.showResetModal}
showPitchModal={!this.props.user || !this.props.user.subscriptionsPlus}
loggedIn={this.props.loggedIn}
showPitchModal={!user || !user.subscriptionsPlus}
loggedIn={loggedIn}
getStats={this.getStats}
selection={this.state.selection}
selectView={this.selectView}
@@ -539,6 +540,7 @@ class Stats extends React.Component {
cancelReset={this.cancelReset}
subscribe={this.subscribe}
signIn={this.signIn}
theme={current_theme}
/>
);
}
@@ -38,6 +38,7 @@ const StatsView = (props) => {
cancelReset,
subscribe,
signIn,
theme
} = props;

const {
@@ -122,6 +123,7 @@ const StatsView = (props) => {
tooltipText={tooltipText}
selectTimeframe={selectTimeframe}
timeframeSelectors={timeframeSelectors}
theme={theme}
/>
<div className="tab-header">
<div className="tab-container">
@@ -28,7 +28,9 @@ import PrioritySupport from './Subscription/PrioritySupport';
class Subscription extends React.Component {
constructor(props) {
super(props);
this.state = { isChecked: (props.current_theme !== 'default') };
this.state = {
theme: this.props.current_theme
};
}

/**
@@ -75,18 +77,16 @@ class Subscription extends React.Component {
return { loading: true };
}

toggleThemes = () => {
const newChecked = !this.state.isChecked;
this.setState({ isChecked: newChecked });
const updated_theme = newChecked ? 'midnight-theme' : 'default';
changeTheme = (updated_theme) => {
this.setState({ theme: updated_theme });
this.props.actions.getTheme(updated_theme).then(() => {
sendMessage('ping', 'theme_change');
});
}

SubscriptionInfoComponent = () => (<SubscriptionInfo subscriptionData={this.parseSubscriptionData()} />);

SubscriptionThemesComponent = () => (<SubscriptionThemes isChecked={this.state.isChecked} toggleThemes={this.toggleThemes} />);
SubscriptionThemesComponent = () => (<SubscriptionThemes theme={this.state.theme} changeTheme={this.changeTheme} />);

PrioritySupportComponent = () => (<PrioritySupport />);

ProTip! Use n and p to navigate between commits in a pull request.