RE-126 - Top Bar.

[bloulidi]

Contents of this pull request:

In this pull request, the top bar was implemented. It contains the following:

• Breadcrumb menu with static data (for now)

• "Change" button which will later be linked with the right panel drawer

• Notification bell

• Dynamic weather indicator with dynamic weather icons (Need to be enable in the code)

• Dynamic geolocation indicator (Need to be enable in the code)

Functionality can be tested by:

1. Enable functions that call the API in TopBar.js (useEffect) to verify is weather, weather icons and geolocation are working.
2. Notification bell opens a small popup that contains static data.
3. Change button is clickable.

⚠️WARNING: For the API's to work, you will need to get an env file from me. I will send it in the discord "files" channel.

How to test:

In your terminal, type the following commands:

• cd RecommendationEngine
• cd RecommendationEngine
• cd FrontEnd
• npm i
• npm start

What it should look like:

[codecov]

Codecov Report

Merging #52 into development will increase coverage by 1.44%.
The diff coverage is 75.67%.

@@              Coverage Diff               @@
##           development     #52      +/-   ##
==============================================
+ Coverage         5.73%   7.17%   +1.44%     
==============================================
  Files               79      80       +1     
  Lines             1745    1756      +11     
  Branches            18      24       +6     
==============================================
+ Hits               100     126      +26     
+ Misses            1643    1630      -13     
+ Partials             2       0       -2     

Flag	Coverage Δ
backend	4.24% <ø> (ø)
frontend	67.07% <75.67%> (+26.22%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
...ionEngine/RecommendationEngine/FrontEnd/src/App.js	100.00% <ø> (ø)
...nd/src/components/Notification/NotificationBell.js	50.00% <50.00%> (ø)
...ionEngine/FrontEnd/src/components/TopBar/TopBar.js	62.50% <62.50%> (ø)
...e/FrontEnd/src/components/Breadcrumb/Breadcrumb.js	100.00% <100.00%> (ø)
...mmendationEngine/FrontEnd/src/components/Layout.js	100.00% <100.00%> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5a4af93...4df6ed0. Read the comment docs.

[AlainJobs]

Right off the bat, your branch is not building nor passing by Travis

[DariushN]

Even though the build isn't working, I still have some comments.
Overall, great work so far! However there's some issues that needs to be resolved;

• First of all, never start a Pull Request without including the tests for it. Right now, merging your branch would decrease the code coverage. The completion of a user story includes the testing of the feature.
• Also, the top bar is using a shade of grey which is a bit too dark in my opinion. It would be nice to have a lighter shade, like in the mockups, to create a better contrast. I would also want to add some more colors, for example, on the weather icon.
• Finally, could we look into using another font for the writing on the top bar? Just a question of personal preferences.

Other than that, once these issues will be resolved or justified, I will give my approval!

[AlainJobs]

Overall

Concerning the components you introduced, Good work!!!!

To work on again

• Reviewing your PR was confusing because you commited the eslint changes. I understand the command was added from the previous PR, thus in dev, but we discussed that we shouldn't commit them. In fact, we changed that command to avoid this in the future. You have 32 changed files instead of 3-4... I ignored the files that had formatting changes and added two reviews. If there's a simple way to revert those eslint commits, it would be neat.
• Add a todo comments "add tests" to your newly added components.
• The breadcrumb should have been done with the material ui components as it comes with more features and the styling is easier.

[bkenza]

The build is failing because of the following error:

I have found the following thread discussing the same issue, so it might be useful for you to take a look :)
jestjs/jest#9292

The top-bar looks great you've done a great job! However, the notification icon renders a bit weirdly on my laptop.
Note: I am using a 13 inch Mac and I have tested on Chrome. This is what it looks like:

So as you can see on the image above, the notification icon gets cutoff!

[bkenza]

Good job on the top-bar, it looks a great! I have left a few comments concerning the code but otherwise everything looks good.

Comments:

• This is not necessarily concerning this specific PR but like Alain mentioned, it would be a good idea to refactor this component using Material-UI.
• Unit testing is necessary before merging into development. If you need help or have any questions please let me know and I can walk you through it step-by-step :)
• Please don't commit any files that you have not explicitly touched during your implementation in order to avoid merge conflicts with the Development branch.
• Also, please refer to my previous comment that mentions a thread to help you deal with the failing Travis build.

[bloulidi]

Even though the build isn't working, I still have some comments.
Overall, great work so far! However there's some issues that needs to be resolved;

• First of all, never start a Pull Request without including the tests for it. Right now, merging your branch would decrease the code coverage. The completion of a user story includes the testing of the feature.
• Also, the top bar is using a shade of grey which is a bit too dark in my opinion. It would be nice to have a lighter shade, like in the mockups, to create a better contrast. I would also want to add some more colors, for example, on the weather icon.
• Finally, could we look into using another font for the writing on the top bar? Just a question of personal preferences.

Other than that, once these issues will be resolved or justified, I will give my approval!

Thanks dariush ! I will make unit and system tests. About the color of the top bar, we decided to go for a darker grey, because the weather icons would fade into the background. I discussed this with the other UI members and they approved. For the top bar font, i like the idea and i will take a look at better options !

[bloulidi]

Overall

Concerning the components you introduced, Good work!!!!

To work on again

• Reviewing your PR was confusing because you commited the eslint changes. I understand the command was added from the previous PR, thus in dev, but we discussed that we shouldn't commit them. In fact, we changed that command to avoid this in the future. You have 32 changed files instead of 3-4... I ignord the files that had formatting changes and left or two reviews. If there's a simple way to revert those eslint commits, it would be neat.
• Add a todo comments "add tests" to your newly added components.
• The breadcrumb should have been done with the material ui components as it comes with more features and the styling is easier.

Thanks for your review Alain ! You are right, the eslint changes are making the code harder to read. I didn't know what it was at first and i just realized later. I will revert to before the eslint changes to make the code more readable. I will add the tests too and for material ui, i will take a look into it later on since we do not have much time for a refactor at the moment.

[bloulidi]

The build is failing because of the following error:

I have found the following thread discussing the same issue, so it might be useful for you to take a look :)
facebook/jest#9292

The top-bar looks great you've done a great job! However, the notification icon renders a bit weirdly on my laptop.
Note: I am using a 13 inch Mac and I have tested on Chrome. This is what it looks like:

So as you can see on the image above, the notification icon gets cutoff!

I will take care of it ! Thank you kenza.

[AlainJobs]

Post Review Overall

Nice work, a lot of stuff had to be changed but it was worth it.
However, these components are subject to change because of some of the stuff we discussed.

Testing: I added a comment regarding the fetching methods used. They should be unit-tested with mock --> jest.mock : Here is a link

• Normally, this has to be done before the merge, but since some branches and user stories are blocked from being PR, you could create a mini-task for it to implement later on.

[bkenza]

Good job on implementing the changes that were requested on your PR comments!

I will approve since we are short on time but I have two comments:

1. Like @AlainJobs mentioned, unit tests should also cover methods in your components.
2. The notification badge does not disappear after notifications are read, so I think you should open a bug report about that.

Other than that, great job!!!

[DariushN]

Looks good to me, great work!