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

hub home and side navigation #167

Closed
wants to merge 4 commits into from
Closed

hub home and side navigation #167

wants to merge 4 commits into from

Conversation

@trickpattyFH20
Copy link
Contributor

@trickpattyFH20 trickpattyFH20 commented Aug 21, 2018

No description provided.

@trickpattyFH20 trickpattyFH20 requested review from IAmThePan and jsignanini Aug 21, 2018
@trickpattyFH20 trickpattyFH20 requested review from zarembsky and ghostery/ghostery as code owners Aug 21, 2018
@zarembsky
Copy link
Contributor

@zarembsky zarembsky commented Aug 22, 2018

How about fancy one-liner in componentWillMount:
({title:window.document.title} = this.state);
instead of:
const { title } = this.state;
window.document.title = title;

@trickpattyFH20
Copy link
Contributor Author

@trickpattyFH20 trickpattyFH20 commented Aug 22, 2018

@zarembsky these should also be moved to the constructor since componentWillMount is deprecated

@trickpattyFH20 trickpattyFH20 force-pushed the feature/hub-home branch 2 times, most recently from 12f154d to ab62b39 Aug 22, 2018
return <div>{title}</div>;
return (
<div>
{title}

This comment has been minimized.

@jsignanini

jsignanini Aug 23, 2018
Member

@trickpattyFH20 let's make sure this is an i18n-ized string

export default TutorialTrustRestrictView;
export function toggleAnalytics(data) {
return {
type: 'TOGGLE_ANALYTICS',

This comment has been minimized.

@jsignanini

jsignanini Aug 23, 2018
Member

@trickpattyFH20 let'a make all action types const on their own constants file.

* Lifecycle Event
*/
componentWillMount() {
const title = 'Ghostery Hub - Home';

This comment has been minimized.

@jsignanini

jsignanini Aug 23, 2018
Member

@trickpattyFH20 this should be i18n-ized.

class TutorialView extends Component {
constructor(props) {
super(props);
this.props.history.push('/tutorial/1');

This comment has been minimized.

@jsignanini

jsignanini Aug 23, 2018
Member

@trickpattyFH20 this should probably live in componentDidMount()?


export function setTutorialNavigation(data) {
return {
type: 'SET_TUTORIAL_NAVIGATION',

This comment has been minimized.

@jsignanini

jsignanini Aug 23, 2018
Member

@trickpattyFH20 this should be a const imported from a constants file.

// };
// return <SteppedNavigation {...navigationProps} />;
// }
// }

This comment has been minimized.

@jsignanini

jsignanini Aug 23, 2018
Member

@trickpattyFH20 let's remove this leftover.

textPrev: t('hub_setup_nav_previous'),
textNext: t('hub_setup_nav_next'),
textDone: t('hub_setup_exit_flow'),
});

This comment has been minimized.

@jsignanini

jsignanini Aug 23, 2018
Member

@trickpattyFH20 should this be on componentDidMount() instead?

constructor(props) {
super(props);

this.state = {
title: ''
title: 'Ghostery Hub - Tutorial Trust'

This comment has been minimized.

@jsignanini

jsignanini Aug 23, 2018
Member

@trickpattyFH20 strings should be i18n-ized.

textPrev: t('hub_setup_nav_previous'),
textNext: t('hub_setup_nav_next'),
textDone: t('hub_setup_exit_flow'),
});

This comment has been minimized.

@jsignanini

jsignanini Aug 23, 2018
Member

@trickpattyFH20 should this be on componentDidMount() instead?

textPrev: false,
textNext: t('hub_setup_nav_next'),
textDone: t('hub_setup_exit_flow'),
});

This comment has been minimized.

@jsignanini

jsignanini Aug 23, 2018
Member

@trickpattyFH20 should this be on componentDidMount() instead?

@trickpattyFH20
Copy link
Contributor Author

@trickpattyFH20 trickpattyFH20 commented Aug 23, 2018

componentDidMount will be deprecated as of React 17
the recommended migration is to move this to the constructor

@trickpattyFH20 trickpattyFH20 force-pushed the feature/hub-home branch from a70f0ea to aa677ba Aug 23, 2018
@IAmThePan IAmThePan force-pushed the feature/hub-home branch from ded1ef9 to 77a1aea Aug 29, 2018
Felix Schmetz Felix Schmetz
@IAmThePan IAmThePan closed this Aug 31, 2018
@IAmThePan
Copy link
Contributor

@IAmThePan IAmThePan commented Aug 31, 2018

This was integrated into the feature/side-navigation pull request

@IAmThePan
Copy link
Contributor

@IAmThePan IAmThePan commented Aug 31, 2018

^^^ Pull Request #179

@IAmThePan IAmThePan deleted the feature/hub-home branch Aug 31, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants