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

#165427095 Design the notification wrapper, view, delete notifications #43

Merged
merged 5 commits into from
May 10, 2019

Conversation

Musigwa-zz
Copy link
Contributor

@Musigwa-zz Musigwa-zz commented Apr 18, 2019

What does this PR do?

  • It adds the notification user interface and handles the notification actions

Description of Task to be completed?

  • Designed the notification UI components
  • Handled get notifications action
  • Added the button to delete the notification
  • It is also possible to navigate to the resource that triggered a notification by clicking on it

How should this be manually tested?

  • After cloning the repository, check out /ft-create-notification-container-165427095 branch
  • Run yarn start to start the application then, you shall see the notification button on the top navigation bar with the number the current user's notifications if any.
  • You can try to dismiss any notification by hovering on it and click the right X button
  • If you click inside the notification message, it will take you to the resource page that triggered it and change the notification's state from unread to read which will be shown by the icon change.

Any background context you want to provide?

  • Beware that I modified a little bit on tests to use test() instead of it() for consistency.
  • I also added a single space between consecutive tests scenarios to improve their readability.

What are the relevant pivotal tracker stories?

  • #165427095

src/components/common/AppBars/navBar.js Outdated Show resolved Hide resolved
src/components/common/AppBars/navBar.js Outdated Show resolved Hide resolved
src/components/common/AppBars/navBar.js Outdated Show resolved Hide resolved
src/__tests__/components/navbar.test.js Outdated Show resolved Hide resolved
src/__tests__/components/navbar.test.js Outdated Show resolved Hide resolved
src/__tests__/components/navbar.test.js Outdated Show resolved Hide resolved
src/__tests__/components/navbar.test.js Outdated Show resolved Hide resolved
Copy link
Contributor

@espoirMur espoirMur left a comment

Choose a reason for hiding this comment

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

@Musigwa! Bro, can you please rename the PR title?
Use the recommended title from engineering playbook.

@Musigwa-zz Musigwa-zz force-pushed the ft-create-notification-container-165427095 branch from 609b361 to 377a3fd Compare April 24, 2019 04:36
@Musigwa-zz Musigwa-zz changed the title [Feature #165427095] design the notification button and wrapper #165427095 Design the notification button and wrapper, view notifications Apr 24, 2019
@Musigwa-zz Musigwa-zz force-pushed the ft-create-notification-container-165427095 branch from 86340e9 to 818383e Compare April 25, 2019 11:24
@Musigwa-zz Musigwa-zz force-pushed the ft-create-notification-container-165427095 branch from ba654f2 to d04d00f Compare April 25, 2019 15:33
Copy link
Contributor

@abayo-luc abayo-luc left a comment

Choose a reason for hiding this comment

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

Well done Mr. @Musigwa ,

  • My first recommendation would be to split the navbar component into small components so that it becomes more maintainable.
  • Minimize the inline style by using class and id.
    I left some comment inside the document. You can check them, and let me know when you are done.

src/components/common/AppBars/Navbar.js Outdated Show resolved Hide resolved
src/components/common/AppBars/Navbar.js Outdated Show resolved Hide resolved
src/components/common/AppBars/Navbar.js Outdated Show resolved Hide resolved
src/redux/actionTypes.js Outdated Show resolved Hide resolved
src/redux/actions/notificationActions.js Outdated Show resolved Hide resolved
@YvesIraguha YvesIraguha had a problem deploying to titan-dev-ah-frontend-st-pr-43 April 26, 2019 08:05 Failure
@Musigwa-zz Musigwa-zz force-pushed the ft-create-notification-container-165427095 branch from 8ceccb7 to d165bad Compare April 26, 2019 08:20
@YvesIraguha YvesIraguha had a problem deploying to titan-dev-ah-frontend-st-pr-43 April 26, 2019 08:20 Failure
@Musigwa-zz Musigwa-zz force-pushed the ft-create-notification-container-165427095 branch from d165bad to a1fccd8 Compare April 26, 2019 12:41
@YvesIraguha YvesIraguha temporarily deployed to titan-dev-ah-frontend-st-pr-43 April 26, 2019 12:41 Inactive
@YvesIraguha YvesIraguha temporarily deployed to titan-dev-ah-frontend-st-pr-43 April 26, 2019 12:46 Inactive
@YvesIraguha YvesIraguha had a problem deploying to titan-dev-ah-frontend-st-pr-43 April 26, 2019 12:51 Failure
@Musigwa-zz Musigwa-zz closed this Apr 26, 2019
@Musigwa-zz Musigwa-zz reopened this Apr 26, 2019
@YvesIraguha YvesIraguha had a problem deploying to titan-dev-ah-frontend-st-pr-43 April 26, 2019 12:56 Failure
@Musigwa-zz Musigwa-zz changed the title #165427095 Design the notification button and wrapper, view notifications #165427095 Design the notification wrapper, view, delete notifications Apr 26, 2019
src/__tests__/__actions__/readArticleAction.test.js Outdated Show resolved Hide resolved
src/__tests__/__actions__/readArticleAction.test.js Outdated Show resolved Hide resolved
src/__tests__/__components__/AppBars/Navbar.test.js Outdated Show resolved Hide resolved
src/__tests__/__components__/AppBars/Navbar.test.js Outdated Show resolved Hide resolved
src/__tests__/__views__/ReadArticle.test.js Outdated Show resolved Hide resolved
src/__tests__/__views__/ReadArticle.test.js Outdated Show resolved Hide resolved
src/__tests__/__views__/ReadArticle.test.js Outdated Show resolved Hide resolved
src/__tests__/__views__/ReadArticle.test.js Outdated Show resolved Hide resolved
src/redux/reducers/notificationReducer.js Outdated Show resolved Hide resolved
@YvesIraguha YvesIraguha had a problem deploying to titan-dev-ah-frontend-st-pr-43 April 30, 2019 07:52 Failure
@YvesIraguha YvesIraguha had a problem deploying to titan-dev-ah-frontend-st-pr-43 April 30, 2019 10:46 Failure
.gitignore Outdated Show resolved Hide resolved
@YvesIraguha YvesIraguha had a problem deploying to titan-dev-ah-frontend-st-pr-43 May 2, 2019 10:00 Failure
@YvesIraguha YvesIraguha had a problem deploying to titan-dev-ah-frontend-st-pr-43 May 3, 2019 07:37 Failure
@YvesIraguha YvesIraguha had a problem deploying to titan-dev-ah-frontend-st-pr-43 May 6, 2019 15:29 Failure
@YvesIraguha YvesIraguha had a problem deploying to titan-dev-ah-frontend-st-pr-43 May 6, 2019 15:34 Failure
@YvesIraguha YvesIraguha had a problem deploying to titan-dev-ah-frontend-st-pr-43 May 6, 2019 15:36 Failure
@Musigwa-zz Musigwa-zz force-pushed the ft-create-notification-container-165427095 branch from d2683da to 2eacd27 Compare May 9, 2019 11:44
@YvesIraguha YvesIraguha had a problem deploying to titan-dev-ah-frontend-st-pr-43 May 9, 2019 11:44 Failure
@YvesIraguha YvesIraguha had a problem deploying to titan-dev-ah-frontend-st-pr-43 May 9, 2019 12:11 Failure
@YvesIraguha YvesIraguha had a problem deploying to titan-dev-ah-frontend-st-pr-43 May 9, 2019 12:23 Failure
Copy link
Contributor

@espoirMur espoirMur left a comment

Choose a reason for hiding this comment

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

LGTM

@Musigwa-zz Musigwa-zz force-pushed the ft-create-notification-container-165427095 branch from 23ab01b to 9a6db40 Compare May 9, 2019 22:41
@YvesIraguha YvesIraguha had a problem deploying to titan-dev-ah-frontend-st-pr-43 May 9, 2019 22:41 Failure
@YvesIraguha YvesIraguha had a problem deploying to titan-dev-ah-frontend-st-pr-43 May 9, 2019 23:33 Failure
@Musigwa-zz Musigwa-zz force-pushed the ft-create-notification-container-165427095 branch from 600e459 to 589b53f Compare May 9, 2019 23:42
@YvesIraguha YvesIraguha had a problem deploying to titan-dev-ah-frontend-st-pr-43 May 9, 2019 23:42 Failure
@YvesIraguha YvesIraguha had a problem deploying to titan-dev-ah-frontend-st-pr-43 May 9, 2019 23:49 Failure
@YvesIraguha YvesIraguha had a problem deploying to titan-dev-ah-frontend-st-pr-43 May 9, 2019 23:50 Failure
@Musigwa-zz Musigwa-zz added the Needs approval Needs to be merged label May 9, 2019
@oesukam oesukam merged commit 8b3450a into develop May 10, 2019
@oesukam oesukam deleted the ft-create-notification-container-165427095 branch May 10, 2019 05:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs approval Needs to be merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants