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

feat(breadcrumb): add a breadcrumb underneath the page header #1815

Merged
merged 51 commits into from
Feb 23, 2020
Merged

feat(breadcrumb): add a breadcrumb underneath the page header #1815

merged 51 commits into from
Feb 23, 2020

Conversation

oliv37
Copy link
Contributor

@oliv37 oliv37 commented Feb 9, 2020

Fixes #1770.

Changes proposed in this pull request:

  • Add a breadcrumb underneath the page header
  • The breadcrumb is updated when the user navigates between pages

Let me know if the breadcrumb created in this PR is what you are expecting, then I will finish all the unit tests

@jsf-clabot
Copy link

jsf-clabot commented Feb 9, 2020

CLA assistant check
All committers have signed the CLA.

@vercel
Copy link

vercel bot commented Feb 9, 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/igmc0ngcr
✅ Preview: https://hospitalrun-frontend-git-fork-oliv37-breadcrumbs.hospitalrun.now.sh

@oliv37 oliv37 changed the title feat(breadcrumb): add a breadcrumb underneath the page header feat(breadcrumb): add a breadcrumb underneath the page header #1770 Feb 9, 2020
@oliv37 oliv37 changed the title feat(breadcrumb): add a breadcrumb underneath the page header #1770 feat(breadcrumb): add a breadcrumb underneath the page header Feb 9, 2020
@oliv37 oliv37 changed the title feat(breadcrumb): add a breadcrumb underneath the page header [WIP] feat(breadcrumb): add a breadcrumb underneath the page header Feb 9, 2020
@jackcmeyer
Copy link
Member

UI Style Feedback: I think that the breadcrumbs should go directly below the page break line, rather than right below the Navbar.

@jackcmeyer jackcmeyer added this to the v2.0.0 milestone Feb 10, 2020
@jackcmeyer jackcmeyer added the 🚀enhancement an issue/pull request that adds a feature to the application label Feb 10, 2020
Comment on lines 7 to 14
const Breadcrumb = () => (
<Switch>
<Route exact path={['/patients/new', '/appointments/new']} component={DefaultBreadcrumb} />
<Route path="/patients/:id" component={PatientBreadcrumb} />
<Route path="/appointments/:id" component={AppointmentBreadcrumb} />
<Route path="*" component={DefaultBreadcrumb} />
</Switch>
)
Copy link
Member

@jackcmeyer jackcmeyer Feb 10, 2020

Choose a reason for hiding this comment

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

I'm wondering if we can combine all of the breadcrumbs into one component.

This breadcrumb component would read from a Provider (defined using the React Context API) or a Redux Slice.

The state of the Provider or Slice would be an array of objects that have two properties: text and location.

The Provider or Slice would have three functions exposed, one to add a breadcrumb, one to remove a breadcrumb, and one to set the breadcrumbs (basically a function to just set the state exactly to what the user passed in).

Each Route Handler (e.g. Patients, ViewPatient, NewPatient) would then add a breadcrumb to the state, and would remove it during the cleanup phase of the useEffect.

This would ensure we don't have any duplicate code, the breadcrumb can be a single component, and we don't have any additional work each time a new route or module is added.

Feel free to ping me on slack with any questions.

Copy link
Member

Choose a reason for hiding this comment

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

For an example of how this might work, checkout https://github.com/HospitalRun/hospitalrun-frontend/pull/1817/files. I'm working through doing something similar but with a button toolbar.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, if my understanding is correct, each Route Component will declare an effect to update the breadcrumb array in the state ?

Copy link
Member

Choose a reason for hiding this comment

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

I think that you have the correct understanding!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I updated my code using your recommendations (without doing the unit tests).
I only handled the / and /patients/** routes, let met know if it's ok for you, then I will finish the tests and handle the /appointments/** routes

Copy link
Contributor Author

@oliv37 oliv37 Feb 15, 2020

Choose a reason for hiding this comment

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

I just finished to implement the whole feature (I still need to improve the code coverage).

It was not as easy as I expected because of the nested routes. I realized the effect of a child component is executed before the effect of its parent. As a result I couldn't rely on the order of the addBreadcrumb function calls to build a valid breadcrumbs array. So I decided to sort the breadcrumbs by their location property before the rendering.

I am also afraid that maintaining the breadcrumbs will be painful in the future, as the number of routes will increase. Each developers should remember to call the hook useAddBreadcrumbs when they add a new route, it can be really cumbersome...

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.

This all looks really great 🔥

package.json Outdated Show resolved Hide resolved
@lauggh
Copy link
Member

lauggh commented Feb 19, 2020

Yes, the "Dashboard" in the breadcrumbs should be linked.

@matteovivona
Copy link
Contributor

@oliv37 can you add this change?

Copy link
Member

@fox1t fox1t left a comment

Choose a reason for hiding this comment

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

Seems good after changes proposed by @tehkapa

@oliv37
Copy link
Contributor Author

oliv37 commented Feb 19, 2020

@lauggh would a functional UX require a "Dashboard" (sort of like a homepage) clickable before "Patients" item?

@tehkapa Do you want me to add the "Dashboard" breadcrumbItem for all the "Patients" routes ("/patients/**") ?

Do I also need to add the "Dashboard" (sort of link to the homepage) before "Appointments" items ?

appointments-breadcrumb

@matteovivona
Copy link
Contributor

@lauggh what do you say? Another note... does it make sense to have breadcrumbs in the Dashboard?
image

@lauggh
Copy link
Member

lauggh commented Feb 19, 2020

No breadcrumbs on the top level. Good catch

#1815 (comment)

@oliv37
Copy link
Contributor Author

oliv37 commented Feb 19, 2020

Still no answer to my first question, Do I need to add "Dashboard" at the beginning of all breadcrumbs (/patients/** , /appointments/** ) ?

@lauggh
Copy link
Member

lauggh commented Feb 19, 2020 via email

@matteovivona matteovivona requested review from jackcmeyer and removed request for lauggh February 20, 2020 08:22
add the dashboard item for all the breacrumbs, don't display the breadcrumb for the Dashboard page
@vercel vercel bot temporarily deployed to Preview February 20, 2020 19:33 Inactive
@oliv37
Copy link
Contributor Author

oliv37 commented Feb 20, 2020

it might be ok now

@matteovivona
Copy link
Contributor

matteovivona commented Feb 20, 2020

@oliv37 can you resolve conflicts? :) thx

great job :)

@matteovivona matteovivona merged commit 878684c into HospitalRun:master Feb 23, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
🚀enhancement an issue/pull request that adds a feature to the application in progress indicates that issue/pull request is currently being worked on v2.x
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add breadcrumbs
6 participants