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.

Sign up for GitHub

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

Jump to bottom

refactor: Cleanup of events, reducers and actions #1382

Merged
merged 6 commits into from
Apr 25, 2018
Merged

refactor: Cleanup of events, reducers and actions #1382

merged 6 commits into from
Apr 25, 2018

Conversation

gregor
Copy link
Contributor

@gregor gregor commented Apr 3, 2018

No description provided.

@gregor gregor force-pushed the bug/WEBAPP-4907 branch 6 times, most recently from e3eefcd to 342fa56 Compare April 4, 2018 14:52
@lipis lipis requested a review from ffflorian April 25, 2018 11:20
atomrc
atomrc reviewed Apr 25, 2018
View reviewed changes
electron/js/certutils.js
hostnameShouldBePinned (hostname) {
return pins.some(pin => pin.url.test(hostname.toLowerCase().trim()));
},
hostnameShouldBePinned: hostname => pins.some(pin => pin.url.test(hostname.toLowerCase().trim())),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a real benefit to this?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the original version is more readable I would say..

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's shorter 😃

AndyLnd
AndyLnd reviewed Apr 25, 2018
View reviewed changes
electron/js/lib/googleAuth.js
@@ -41,12 +40,13 @@ const _authorizeApp = (url) => {
setImmediate(() => {
const title = win.getTitle();

const [, , returnValue] = title.split(/[ =]/);
Copy link
Contributor

@AndyLnd AndyLnd Apr 25, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is more cryptic than const returnValue = title.split(/[ =]/)[2];

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But it fails as soon as you enable magic number checking

ffflorian
ffflorian previously requested changes Apr 25, 2018
View reviewed changes
electron/js/lib/googleAuth.js
@@ -41,12 +40,13 @@ const _authorizeApp = (url) => {
setImmediate(() => {
const title = win.getTitle();

const [, , returnValue] = title.split(/[ =]/);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

const returnValue = title.split(/[ =]/)[2];

electron/js/lib/openGraph.js Outdated
@@ -41,13 +41,13 @@ const _fetchImageAsBase64 = (url) => {
});
};

const _fetchOpenGraphData = (url) => {
const fetchOpenGraphData = (url) => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

const fetchOpenGraphData = url => {

electron/js/lib/openGraph.js Outdated
const _getOpenGraphData = (url, callback) => {
return _fetchOpenGraphData(url)
const getOpenGraphData = (url, callback) => {
return fetchOpenGraphData(url)
.then((meta) => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

.then(meta => {

electron/js/lib/openGraph.js Outdated
return _fetchImageAsBase64(imageUrl)
.then((uri) => _updateMetaDataWithImage(meta, uri))
return fetchImageAsBase64(imageUrl)
.then((uri) => updateMetaDataWithImage(meta, uri))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

.then(uri => updateMetaDataWithImage(meta, uri))

electron/renderer/src/actions/index.js Outdated
id,
type: DELETE_ACCOUNT,
type: UPDATE_ACCOUNT_BADGE,
};
};
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

export const updateAccountBadge = (id, count) => ({
  count,
  id,
  type: UPDATE_ACCOUNT_BADGE,
});

Same for the other functions in this file.

electron/renderer/src/actions/index.js Outdated
export const setAccountContextHidden = () => {
return {
type: 'HIDE_CONTEXT_MENUS',
type: HIDE_CONTEXT_MENUS,
};
};
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

export const setAccountContextHidden = () => ({
  type: HIDE_CONTEXT_MENUS,
});

electron/renderer/src/components/Webview.js Outdated
const is_visible = this.props.visible === true;
if (is_visible) {
const isVisible = this.props.visible === true;
if (isVisible) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

if (this.props.visible === true) {

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

or just if (this.props.visible)?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I prefer the explicit test.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's redundant since the variable name is already very clear.. https://softwareengineering.stackexchange.com/questions/12807/make-a-big-deal-out-of-true

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

plus it's shorter :)

electron/renderer/src/lib/eventType.js Outdated
export const ACTION = {
NOTIFICATION_CLICK: 'EVENT_TYPE.ACTION.NOTIFICATION_CLICK',
};
export const LIFECYCLE = {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

};

export const LIFECYCLE = {

electron/renderer/src/lib/localStorage.js Outdated
@@ -33,7 +32,7 @@ export const loadState = () => {
export const saveState = (state) => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

export const saveState = state => {

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

these things will be fixed automagically once the #1058 will be fixed and eventually Prettier (#1035)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, yes, eventually...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe even tomorrow :) as @gregor has some ideas :) Robots will do their job, eventually..

electron/renderer/src/reducers/accountReducer.js Outdated
teamID: undefined,
userID: undefined,
visible: true,
};
}
};
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

const createAccount = sessionId => ({
  accentID: undefined,
  badgeCount: 0,
  id: uuid(),
  lifecycle: undefined,
  name: undefined,
  picture: undefined,
  sessionID: sessionId,
  teamID: undefined,
  userID: undefined,
  visible: true,
});

Yserz
Yserz reviewed Apr 25, 2018
View reviewed changes
electron/renderer/src/reducers/accountReducer.js Outdated

const accounts = (state = [createAccount()], action) => {
const accountReducer = (state = [createAccount()], action) => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

Yserz
Yserz reviewed Apr 25, 2018
View reviewed changes
electron/renderer/static/webview-preload.js
id,
config.LOG_FILE_NAME
);
const logFilePath = path.join(app.getPath('userData'), 'logs', id, config.LOG_FILE_NAME);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

WDYT about making the logs dir name a constant?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Like the idea but I don't want to add more stuff to this PR than we already have.

Yserz
Yserz reviewed Apr 25, 2018
View reviewed changes
electron/renderer/static/webview-preload.js
'img',
'notification.png'
);
global.notification_icon = path.join(app.getAppPath(), 'img', 'notification.png');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

WDYT about making the img dir name a constant?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See above

@gregor gregor dismissed ffflorian’s stale review April 25, 2018 15:01

Addressed feedback where agreed

@gregor gregor merged commit f705c0a into master Apr 25, 2018
@gregor gregor deleted the bug/WEBAPP-4907 branch April 25, 2018 15:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ffflorian ffflorian ffflorian left review comments

@Yserz Yserz Yserz left review comments

@AndyLnd AndyLnd AndyLnd left review comments

@atomrc atomrc atomrc left review comments

@lipis lipis lipis approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@gregor @lipis @atomrc @AndyLnd @Yserz @ffflorian