Skip to content
This repository has been archived by the owner on Jan 9, 2023. It is now read-only.

fix(frontend): update navbar component to match new format #1782

Merged
merged 3 commits into from
Feb 3, 2020

Conversation

ocBruno
Copy link
Contributor

@ocBruno ocBruno commented Feb 2, 2020

BREAKING CHANGE: update components package changing some interfaces

fix #1781

Changes proposed in this pull request:

  • update components package
  • update navbar to match changes

https://github.com/HospitalRun/components
32.2

BREAKING CHANGE: update components package changing some interfaces

fix HospitalRun#1781
@vercel
Copy link

vercel bot commented Feb 2, 2020

This pull request is being automatically deployed with ZEIT Now (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://zeit.co/hospitalrun/hospitalrun-frontend/66b9aisfo
✅ Preview: https://hospitalrun-frontend-git-fork-ocbruno-fix-update-navbar.hospitalrun.now.sh

@jackcmeyer jackcmeyer added 🐛bug issue/pull request that documents/fixes a bug dependencies Pull requests that update a dependency file labels Feb 2, 2020
@jackcmeyer jackcmeyer added this to the v2.0.0 milestone Feb 2, 2020
@ocBruno
Copy link
Contributor Author

ocBruno commented Feb 2, 2020

Hey @jackcmeyer
To be able to finish resolving this efficiently, I need to first create a pull request to update the components library to add className identifiers to the navitems to be able to properly test.
I will create a pull request for that before finishing up this update.

@jackcmeyer
Copy link
Member

Hey @jackcmeyer
To be able to finish resolving this efficiently, I need to first create a pull request to update the components library to add className identifiers to the navitems to be able to properly test.
I will create a pull request for that before finishing up this update.

@ocBruno doesn't NavItems already have a className https://github.com/HospitalRun/components/blob/9a492170d88d103b7d00454392cd77923ce2f2d3/src/components/Navbar/interfaces.tsx#L4?

src/components/Navbar.tsx Outdated Show resolved Hide resolved
src/components/Navbar.tsx Outdated Show resolved Hide resolved
Copy link
Member

@jackcmeyer jackcmeyer left a comment

Choose a reason for hiding this comment

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

Code loks good, just need some tests to get update

src/components/Navbar.tsx Outdated Show resolved Hide resolved
@ocBruno
Copy link
Contributor Author

ocBruno commented Feb 2, 2020

Hey @jackcmeyer
To be able to finish resolving this efficiently, I need to first create a pull request to update the components library to add className identifiers to the navitems to be able to properly test.
I will create a pull request for that before finishing up this update.

@ocBruno doesn't NavItems already have a className https://github.com/HospitalRun/components/blob/9a492170d88d103b7d00454392cd77923ce2f2d3/src/components/Navbar/interfaces.tsx#L4?

Ahh yes, I was going to add a default identifiers such as nav-header and nav-icon but now that you mentioned the built in one i'm not sure we need to, I will add classes to each item and test them with those.
** Edit **
I realized there could be some refactoring in the components library to improve test structure in the projects. There is some unnecessary nesting such as in the navheader.

also refactored the test structure to improve organization, speed and remove code repetition

fix HospitalRun#1782
@ocBruno
Copy link
Contributor Author

ocBruno commented Feb 2, 2020

Hey @jackcmeyer
To be able to finish resolving this efficiently, I need to first create a pull request to update the components library to add className identifiers to the navitems to be able to properly test.
I will create a pull request for that before finishing up this update.

@ocBruno doesn't NavItems already have a className https://github.com/HospitalRun/components/blob/9a492170d88d103b7d00454392cd77923ce2f2d3/src/components/Navbar/interfaces.tsx#L4?

Hey @jackcmeyer,

finished updating the tests and suggested a new structure with some improvements we could apply to other tests!

ocBruno added a commit to ocBruno/hospitalrun-frontend that referenced this pull request Feb 2, 2020
missing tests from precommit hook to be able to commit changes

fix HospitalRun#1782
ocBruno added a commit to ocBruno/hospitalrun-frontend that referenced this pull request Feb 2, 2020
@matteovivona matteovivona merged commit fe0fb6d into HospitalRun:master Feb 3, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
🐛bug issue/pull request that documents/fixes a bug dependencies Pull requests that update a dependency file
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Property 'brand' does not exist on type IntrinsicAttributes
3 participants