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

Bottom navbar and new top navbar #902

Merged
merged 31 commits into from
Feb 15, 2024
Merged

Bottom navbar and new top navbar #902

merged 31 commits into from
Feb 15, 2024

Conversation

Process-ing
Copy link
Contributor

@Process-ing Process-ing commented Aug 9, 2023

Closes #819, closes #1066
The bottom navbar is based on the new redesign of the app, so the pages don't fully match. I also took the freedom to refactor all the pages with the new top navbar style, although it will be probably scrapped for most pages

Review checklist

  • Terms and conditions reflect the current change
  • Contains enough appropriate tests
  • If aimed at production, writes a new summary in whatsnew/whatsnew-pt-PT
  • Properly adds an entry in changelog.md with the change
  • If PR includes UI updates/additions, its description has screenshots
  • Behavior is as expected
  • Clean, well-structured code

@Process-ing Process-ing self-assigned this Aug 9, 2023
@Process-ing Process-ing marked this pull request as draft August 9, 2023 18:06
Copy link
Member

@bdmendes bdmendes left a comment

Choose a reason for hiding this comment

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

I like the implementation quite a lot.
Two nitpicks:

  1. Does it make sense that the navbar items are in the utils directory?
  2. We are now referencing routes in even more places: besides on card tap actions and drawer links, we now have bottom nav bar links. Can we encapsulate a route in a enum with a string?

@Process-ing
Copy link
Contributor Author

Process-ing commented Aug 10, 2023

  1. If we think that the navigation drawer items are in the utils directory too, I think it makes sense.
  2. I totally agree with this. I think we shouldn't have stored the routes' names with the drawer items in the first place, though it didn't seem relevant for the objective of this issue. I can do this in this pr too if you'd like.

@Process-ing Process-ing requested review from bdmendes and removed request for bdmendes August 11, 2023 16:04
@codecov
Copy link

codecov bot commented Aug 11, 2023

Codecov Report

Merging #902 (142741e) into develop (c208f9e) will increase coverage by 1%.
The diff coverage is 34%.

Additional details and impacted files
@@          Coverage Diff           @@
##           develop   #902   +/-   ##
======================================
+ Coverage       16%    16%   +1%     
======================================
  Files          225    227    +2     
  Lines         6851   6839   -12     
======================================
+ Hits          1053   1062    +9     
+ Misses        5798   5777   -21     

@bdmendes
Copy link
Member

@Process-ing is this ready? If so, please undraft

@Process-ing Process-ing marked this pull request as ready for review August 13, 2023 16:23
Copy link
Member

@bdmendes bdmendes left a comment

Choose a reason for hiding this comment

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

(dummy request changes to avoid getting merged until the release)

@bdmendes
Copy link
Member

From last design meeting, we'll probably end up with:

  • Personal Area
  • Academic Path (Schedule, Exams, Courses/Course Units)
  • Faculty (Maps, Library, Utils)
  • Restaurants
  • Transports

@Process-ing Process-ing marked this pull request as ready for review February 3, 2024 15:35
@bdmendes
Copy link
Member

bdmendes commented Feb 3, 2024

@Process-ing can you update the PR screenshots with the new design?

@bdmendes bdmendes dismissed Sirze01’s stale review February 14, 2024 19:55

Bug unrelated to this PR

@limwa
Copy link
Member

limwa commented Feb 15, 2024

LGTM 🚀

Copy link
Member

@LuisDuarte1 LuisDuarte1 left a comment

Choose a reason for hiding this comment

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

🚀

@LuisDuarte1 LuisDuarte1 merged commit 1306792 into develop Feb 15, 2024
6 checks passed
@LuisDuarte1 LuisDuarte1 deleted the feature/bottom-navbar branch February 15, 2024 15:45
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.

Refactor top bar Bottom nav bar
5 participants