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

Remove Refactoring notes from feature/cleanup and address PR comments #566

Merged
merged 6 commits into from Jun 15, 2020
Merged
Changes from all commits
Commits
File filter
Filter file types
Jump to
Jump to file
Failed to load files.

Always

Just for now

@@ -40,7 +40,6 @@ module.exports = {
rules: {
'arrow-parens': [2, 'as-needed', { 'requireForBlockBody': true }],
'camelcase': [0],
'class-methods-use-this': [1],
'comma-dangle': [2, {
'arrays': 'only-multiline',
'objects': 'only-multiline',
@@ -54,7 +53,6 @@ module.exports = {
'lines-between-class-members': [1],
'max-len': [0],
'newline-per-chained-call': [0, { 'ignoreChainWithDepth': 2 }],
'no-mixed-operators': [1],
'no-nested-ternary': [0],
'no-param-reassign': ['error', {
props: true,
@@ -72,28 +70,22 @@ module.exports = {
]
}],
'no-plusplus': [0],
'no-prototype-builtins': [0],
'no-restricted-syntax': [1],
'no-prototype-builtins': [0], // Ignored because of hasOwnProperty calls.
'no-tabs': [0],
'no-underscore-dangle': [0],
'no-unused-vars': [1],
'no-useless-escape': [1],
'operator-linebreak': [0],
'prefer-object-spread': [1],
'space-before-function-paren': [2, 'never'],

// Plugin: Import
'import/no-cycle': [0],
'import/prefer-default-export': [1],

// Plugin: React
'react/destructuring-assignment': [1],
'react/static-property-placement': [0],
'react/jsx-curly-newline': [0],
'react/jsx-indent': [1, 'tab'],
'react/jsx-indent-props': [1, 'tab'],
'react/jsx-props-no-spreading': [1],
'react/no-access-state-in-setstate': [1],
'react/no-danger': [0],
'react/prop-types': [0],
'react/jsx-fragments': [1, 'element'],
@@ -161,11 +161,10 @@ const Click2PlayContentScript = (function(win, doc) {
if (message) {
// Dequeue C2P data stored while the script injection was taking place
const messageKeys = Object.keys(message);
for (let i = 0; i < messageKeys.length; i++) {
const app_id = messageKeys[i];
messageKeys.forEach((app_id) => {
applyC2P(app_id, message[app_id].data, message[app_id].html);
delete message[app_id];
}
});
}
}

This file was deleted.

@@ -148,8 +148,7 @@ export default class Panel extends React.Component {
}

render() {
const { panel } = this.props;
const { cliqzModuleData } = this.state;
const { panel, cliqzModuleData } = this.state;
return (
<div>
<div className={`chart-wrapper ${this.siteProps.isPaused ? 'paused' : ''}`}>
@@ -24,14 +24,13 @@ export default class ChartSVG extends React.Component {
}

increaseN = () => {
const { paths } = this.props;
const { nItem } = this.state;
let currentN = nItem;
if (currentN < paths.length) {
this.setState({
nItem: currentN += 1,
});
}
this.setState((prevState) => {
const { paths } = this.props;
if (prevState.nItem < paths.length) {
return { nItem: prevState.nItem + 1 };
}
return null;
});
}

render() {
@@ -43,12 +43,7 @@ export default class DotsMenu extends React.Component {

/* Toggle menu */
dotsButtonClicked = () => {
const { opening } = this.state;
const currentState = opening;

this.setState({
opening: !currentState,
});
this.setState(prevState => ({ opening: !prevState.opening }));
}

render() {
@@ -51,16 +51,14 @@ export default class FixedMenu extends React.Component {
switch (type) {
case 'enable_anti_tracking': {
const categories = Object.keys(this.antiTrackingData);
for (let i = 0; i < categories.length; i++) {
const category = categories[i];
categories.forEach((category) => {
const apps = Object.keys(this.antiTrackingData[category]);
for (let j = 0; j < apps.length; j++) {
const app = apps[j];
apps.forEach((app) => {
if (this.antiTrackingData[category][app] === 'unsafe') {
total++;
}
}
}
});
});
return total;
}
case 'enable_ad_block':
@@ -79,14 +77,10 @@ export default class FixedMenu extends React.Component {
}

toggleMenu = () => {
const { open } = this.state;
const currentState = open;
this.setState({
open: !currentState,
});
this.setState(prevState => ({ open: !prevState.open }));
}

updateHeadeText = (text) => {
updateHeaderText = (text) => {
const textToShow = text || FixedMenu.defaultHeaderText;

this.setState({
@@ -106,7 +100,7 @@ export default class FixedMenu extends React.Component {
<li className="menuItem">
<MenuItem
active={panel.enable_anti_tracking}
updateHeadeText={this.updateHeadeText}
updateHeaderText={this.updateHeaderText}
type="anti_tracking"
title="Enhanced Anti-Tracking"
numData={this.getCount('enable_anti_tracking')}
@@ -117,7 +111,7 @@ export default class FixedMenu extends React.Component {
<li className="menuItem">
<MenuItem
active={panel.enable_ad_block}
updateHeadeText={this.updateHeadeText}
updateHeaderText={this.updateHeaderText}
type="ad_block"
title="Enhanced Ad Blocking"
numData={this.getCount('enable_ad_block')}
@@ -128,7 +122,7 @@ export default class FixedMenu extends React.Component {
<li className="menuItem">
<MenuItem
active={panel.enable_smart_block}
updateHeadeText={this.updateHeadeText}
updateHeaderText={this.updateHeaderText}
type="smart_block"
title="Smart Blocking"
numData={this.getCount('enable_smart_block')}
@@ -24,21 +24,21 @@ export default class MenuItem extends React.Component {
}

menuItemClicked = () => {
const { updateHeadeText, title } = this.props;
const { updateHeaderText, title } = this.props;
this.setState({
opening: true,
});

updateHeadeText(title);
updateHeaderText(title);
}

closeButtonClicked = () => {
const { updateHeadeText } = this.props;
const { updateHeaderText } = this.props;
this.setState({
opening: false,
});

updateHeadeText('');
updateHeaderText('');
}

switcherClicked = () => {
@@ -26,18 +26,6 @@ import { updateSummaryBlockingCount } from '../utils/blocking';
class Blocking extends React.Component {
static contextType = DynamicUIPortContext;

/**
* Refactoring UNSAFE_componentWillMount into Constructor
* Stats:
* Constructor runtime before refactor: 0.038ms
* Constructor + UNSAFE_componentWillMount runtime before refactor: 0.333ms
* Constructor runtime after refactor: 0.129ms
*
* Notes:
* calling buildBlockingClasses and computeSiteNotScanned in the constructor takes 0.018ms.
*
* Conclusion: Refactor using constructor as the added computation is minimal
*/
constructor(props) {
super(props);

@@ -48,9 +48,6 @@ class BlockingHeader extends React.Component {

/**
* Lifecycle event
* Refactor Notes:
* Refactor UNSAFE_componentWillReceiveProps using getDerivedStateFromProps
* because we are only manipulating state.
*/
static getDerivedStateFromProps(prevProps, prevState) {
return BlockingHeader.updateBlockAll(prevProps.categories, prevState.fromHere);
@@ -27,20 +27,6 @@ import { renderKnownTrackerButtons, renderUnknownTrackerButtons } from './tracke
class Tracker extends React.Component {
static contextType = ThemeContext;

/**
* Refactoring UNSAFE_componentWillMount into Constructor
* Stats:
* Constructor runtime before refactor: 0.037ms
* Constructor + UNSAFE_componentWillMount runtime before refactor: 0.415ms
* Constructor runtime after refactor: 0.215ms
*
* Refactoring UNSAFE_componentWillMount into componentDidMount
* Stats:
* Constructor runtime after refactor: 0.020ms
* Constructor + componentDidMount runtime after refactor: 14.205ms
*
* Conclusion: Refactor using componentDidMount
*/
constructor(props) {
super(props);
this.state = {
@@ -153,7 +153,7 @@ class DonutGraph extends React.Component {
return;
}

// componentWillReceiveProps gets called many times during page load as new trackers or unsafe data points are found
// componentDidUpdate gets called many times during page load as new trackers or unsafe data points are found
This conversation was marked as resolved by wlycdgr

This comment has been minimized.

@wlycdgr

wlycdgr Jun 11, 2020
Member

Nice catch

// so only compare tracker totals if we don't already have to redraw anyway as a result of the cheaper checks above
const prevTrackerTotal = prevCategories.reduce((total, category) => total + category.num_total, 0);
const trackerTotal = categories.reduce((total, category) => total + category.num_total, 0);
@@ -22,23 +22,6 @@ import Rewards from '../containers/RewardsContainer';
* @memberOf PanelClasses
*/
class Detail extends React.Component {
/**
* Refactoring UNSAFE_componentWillMount into Constructor
* Stats:
* Constructor runtime before refactor: 0.085ms
* Constructor + UNSAFE_componentWillMount runtime before refactor: 0.345ms
* Constructor runtime after refactor: 0.163ms
*
* Refactoring UNSAFE_componentWillMount into componentDidMount
* Stats:
* Constructor runtime with no componentDidMount: 0.163ms
* Constructor runtime with componentDidMount: 0.078ms
* Constructor + componentDidMount runtime: 8.313ms
* Notes:
* Noticably slower when refactoring using componentDidMount
*
* Conclusion: Refactor using constructor
*/
constructor(props) {
super(props);

@@ -42,22 +42,6 @@ class Settings extends React.Component {
});
}, 3000)

/**
* Refactoring UNSAFE_componentWillMount into Constructor
* Stats:
* Constructor runtime before refactor: 0.145ms
* Constructor + UNSAFE_componentWillMount runtime before refactor: 0.330ms
* Constructor runtime after refactor: 0.144ms
*
* Refactoring UNSAFE_componentWillMount into componentDidMount
* Stats:
* Constructor runtime with no componentDidMount: 0.123ms
* Constructor runtime with componentDidMount: 0.147ms
*
* Notes: Negligible difference using componentDidMount.
*
* Conclusion: Refactor using constructor to avoid re-render
*/
constructor(props) {
super(props);
this.state = {
@@ -19,23 +19,6 @@ import moment from 'moment/min/moment-with-locales.min';
* @memberOf SettingsComponents
*/
class GeneralSettings extends React.Component {
/**
* Refactoring UNSAFE_componentWillMount into Constructor
* Stats:
* Constructor runtime before refactor: 0.026ms
* Constructor + UNSAFE_componentWillMount runtime before refactor: 2.410ms
* Constructor runtime after refactor: 1.631ms
*
* Refactoring UNSAFE_componentWillMount into componentDidMount
* Stats:
* Constructor runtime with no componentDidMount: 0.208ms
* Constructor runtime with componentDidMount: 0.074ms
*
* Notes:
* updateDbLastUpdated takes ~2ms to run the firt time and then 0.139ms subsequent times.
*
* Conclusion: Refactor using componentDidMount as to not do computations in the constructor
*/
constructor(props) {
super(props);
this.state = {
@@ -184,8 +184,6 @@ class TrustAndRestrict extends React.Component {
const {
menu, trustedValue, currentWarning, restrictedValue
} = this.state;
const trusted_sites = site_whitelist;
const restricted_sites = site_blacklist;
return (
<div className="s-trust-restrict-panel s-tabs-panel">
<div className="row">
@@ -212,8 +210,8 @@ class TrustAndRestrict extends React.Component {
<div className={`${currentWarning ? '' : 's-invisible '}s-callout`}>{currentWarning}</div>
</div>
</div>
{ trusted_sites && trusted_sites.length > 0 &&
<Sites sites={trusted_sites} listType="whitelist" updateSitePolicy={actions.updateSitePolicy} />
{ site_whitelist && site_whitelist.length > 0 &&
<Sites sites={site_whitelist} listType="whitelist" updateSitePolicy={actions.updateSitePolicy} />
}
</div>
<div className={`${menu.showRestrictedSites ? '' : 's-hide '}s-sites-pane`}>
@@ -227,8 +225,8 @@ class TrustAndRestrict extends React.Component {
<div className={`${currentWarning ? '' : 's-invisible '}s-callout`}>{currentWarning}</div>
</div>
</div>
{ restricted_sites && restricted_sites.length > 0 &&
<Sites sites={restricted_sites} listType="blacklist" updateSitePolicy={actions.updateSitePolicy} />
{ site_blacklist && site_blacklist.length > 0 &&
<Sites sites={site_blacklist} listType="blacklist" updateSitePolicy={actions.updateSitePolicy} />
}
</div>
</div>
@@ -82,7 +82,7 @@ class Summary extends React.Component {
}

/**
* Calculates total tracker latency and sets it to state
* Calculates total tracker latency
* @param {Object} props Summary's props, either this.props or nextProps.
*/
static _computeTrackerLatency(props) {
@@ -268,17 +268,6 @@ export default (state = initialState, action) => {
setTheme(document);
return { ...state, current_theme: initialState.current_theme };
}
// @TODO?
// case LOGOUT_SUCCESS: {
// const notificationAction = {
// payload: {
// text: 'Logged out successfully.',
// classes: 'success',
// }
// };
// const updated = _showNotification(state, notificationAction);
// return Object.assign({}, state, updated);
// }
case RESET_PASSWORD_SUCCESS: {
const notificationAction = {
payload: {
ProTip! Use n and p to navigate between commits in a pull request.