Skip to content

Conversation

chownces
Copy link
Contributor

Description

  • Fix CSS issues from react-router upgrade
  • Improve readability of navbar component

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • Code quality improvements

How to test

Checklist

  • I have tested this code

@chownces chownces requested a review from RichDom2185 June 16, 2023 10:34
@chownces chownces marked this pull request as draft June 16, 2023 10:35
@chownces chownces marked this pull request as ready for review June 16, 2023 13:08
@coveralls
Copy link

coveralls commented Jun 16, 2023

Pull Request Test Coverage Report for Build 5290926602

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

Details

  • 47 of 56 (83.93%) changed or added relevant lines in 3 files are covered.
  • 6 unchanged lines in 2 files lost coverage.
  • Overall coverage decreased (-0.04%) to 36.856%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/commons/navigationBar/NavigationBar.tsx 34 43 79.07%
Files with Coverage Reduction New Missed Lines %
src/commons/navigationBar/NavigationBar.tsx 1 73.12%
src/commons/utils/Constants.ts 5 59.43%
Totals Coverage Status
Change from base Build 5290924072: -0.04%
Covered Lines: 5192
Relevant Lines: 13277

💛 - Coveralls

Copy link
Member

@RichDom2185 RichDom2185 left a comment

Choose a reason for hiding this comment

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

Looks good! The codebase is so much more readable now 😄 I only have some minor comments as follows.

Thanks for this long-overdue refactor!

Copy link
Member

@RichDom2185 RichDom2185 left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@RichDom2185 RichDom2185 enabled auto-merge (squash) June 19, 2023 05:32
@RichDom2185 RichDom2185 merged commit 785ef4e into master Jun 19, 2023
@RichDom2185 RichDom2185 deleted the refactor-navbar branch June 19, 2023 05:35
RichDom2185 added a commit to NUS-CS1101S/cadet-frontend that referenced this pull request Jul 15, 2023
* Refactor navbar

* Update navbar snapshots

* Refactor AcademyNavigationBar

* Fix snapshots

* Fix PR comments

* Fix snapshots

---------

Co-authored-by: Richard Dominick <34370238+RichDom2185@users.noreply.github.com>
RichDom2185 added a commit to NUS-CS1101S/cadet-frontend that referenced this pull request Aug 15, 2023
* Refactor navbar

* Update navbar snapshots

* Refactor AcademyNavigationBar

* Fix snapshots

* Fix PR comments

* Fix snapshots

---------

Co-authored-by: Richard Dominick <34370238+RichDom2185@users.noreply.github.com>
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