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

#166816075 user notification opt-in/opt-out #46

Merged
merged 1 commit into from
Sep 3, 2019

Conversation

devPinheiro
Copy link
Contributor

@devPinheiro devPinheiro commented Aug 29, 2019

What does this PR do?

  • It contains a feature for setting user preference on notifications

Description of Task to be completed?

  • Update edit profile modal
  • Add notification toggle
  • Write unit tests

How should this be manually tested?

  • After cloning the repo, cd into the root folder

  • Install all necessary dependencies by running

    npm install

  • Launch the node server with

    npm run dev

  • Using a web browser visit the link below:

    localhost:8080/

  • To run local tests using Jest

    npm test

Any background context you want to provide?

  • N/A

Screenshot 2019-08-30 at 6 39 35 AM

What are the relevant pivotal tracker stories?

  • PT Story link - [#166816075] (https://www.pivotaltracker.com/story/show/166816075)

@devPinheiro devPinheiro requested review from dharmykoya and tunedev and removed request for dharmykoya August 29, 2019 19:09
@devPinheiro devPinheiro force-pushed the ft-notification-opt-in-out-166816075 branch from 1b35ee5 to 071bf19 Compare August 30, 2019 05:39
Copy link
Contributor

@fxola fxola left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@dharmykoya dharmykoya left a comment

Choose a reason for hiding this comment

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

asides the commented codes.... the PR looks good to me

@@ -61,7 +61,7 @@ describe('<ProfileNavbar />', () => {
</BrowserRouter>
</Provider>
);
const editButton = profileNav.find('.btn-navbar').simulate('click');
// const editButton = profileNav.find('.btn-navbar').simulate('click');
Copy link
Contributor

Choose a reason for hiding this comment

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

please do remove this commented code

}
}
};
// const props = {
Copy link
Contributor

Choose a reason for hiding this comment

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

remove this commented codes too... thanks

.find('.article-body StarRatingComponent label')
.at(2)
.simulate('click');
// const instance = readArticle.instance();
Copy link
Contributor

Choose a reason for hiding this comment

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

remove this as well

@devPinheiro devPinheiro force-pushed the ft-notification-opt-in-out-166816075 branch from 071bf19 to b5ce475 Compare September 3, 2019 16:14
update edit profile component
write tests
[Finishes #166816075]
@devPinheiro devPinheiro force-pushed the ft-notification-opt-in-out-166816075 branch from b5ce475 to d511571 Compare September 3, 2019 16:27
@dharmykoya dharmykoya merged commit 6a57524 into develop Sep 3, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants