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

feat(navbar): refactor navbar to support horizontal nav item placement and more #228

Merged
merged 32 commits into from
Jan 30, 2020
Merged

feat(navbar): refactor navbar to support horizontal nav item placement and more #228

merged 32 commits into from
Jan 30, 2020

Conversation

ocBruno
Copy link
Contributor

@ocBruno ocBruno commented Jan 18, 2020 •
edited
Loading

fix #225

Changes proposed in this pull request:

  • Refactor navbar function components
  • Abstract nav items to NavItem interface with a string type property that is the kebab-case version of the component NavIcon | NavHeader | NavLink | NavSearch | NavLinkList
  • Support custom horizontal component position
  • Suggest new default theme and dark theme color
  • Refactor tests to improve naming convention
  • Add className prop to each component which corresponds to its inner top level element's className to better support customization
  • Add example with icons
  • Design/layout changes in stories

I'm still willing to fix anything in this issue before approval if necessary or increment any changes. Also willing to update the frontend navbar to support these changes after approval!

@ocBruno ocBruno closed this Jan 18, 2020
@ocBruno ocBruno reopened this Jan 18, 2020
@jackcmeyer jackcmeyer added the enhancement New feature or request label Jan 18, 2020
@jackcmeyer jackcmeyer added this to the v1.0.0 milestone Jan 18, 2020
jackcmeyer
jackcmeyer reviewed Jan 20, 2020
View reviewed changes
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.

Could you explain these changes a little bit more? I'm conflicted whether or not they are necessary. This definitely gives us a more flexible design, however, I'm not sure if that is something that we want. Since we know that our Navbar is always going to have some sort of brand, some sort of search then will have elements to the right of the Brand, and to the right of Search. If that is the case, then it seems like the original design ensures that the user of the component always gets the elements in the right place.

@ocBruno
Copy link
Contributor Author

ocBruno commented Jan 20, 2020

Could you explain these changes a little bit more? I'm conflicted whether or not they are necessary. This definitely gives us a more flexible design, however, I'm not sure if that is something that we want. Since we know that our Navbar is always going to have some sort of brand, some sort of search then will have elements to the right of the Brand, and to the right of Search. If that is the case, then it seems like the original design ensures that the user of the component always gets the elements in the right place.

@jackcmeyer I don't think they are necessary but I think considering the original layouts can still be applied, the issue is resolved and now there are new positioning possibilities it gives the framework user more freedom.

@ocBruno
Copy link
Contributor Author

ocBruno commented Jan 23, 2020 •
edited
Loading

I'm not sure if this is a storybook bug or this is solvable with a feature but the children property in LinkList items aren't rendering the correct code preview. When I comment out the link list, everything renders correctly so I'm not sure if anyone knows of a workaround, if I get some extra time I'll create a reproducible example and open a new issue in storybook repo considering I didn't find anything related.

I added a new custom example and refactored the brand component into an icon and a header component to support horizontal positioning, improve reusability and separation of concerns.
I also added a couple suggestions considering the colors. I suggest making the default theme light considering the color white's association with hospitals and I believe it fits better for the default theme.

In hospital environments, a colour preference for “whiteness” or pale
shades was found among patients with its connotations of cleanliness and
hygiene [2.14].
Reference

I also suggested a change in the dark color, reducing brightness and saturation to cause less eye strain while maintaining readibility.

@ocBruno
fix(navbar): fix tests, refactor brand component to icon and header,
91ce73e 
fix tests, refactor brand component into icon and header to support horizontal positioning, improve
reusability and separation of concerns, add new custom example

fix #228
@jackcmeyer
Copy link
Member

@ocBruno could we make the text of the nav links match the color of the brand and then be their current color when they are hovered over?

Plus, can the search bar remain to the left?

Can you provide a storybook example of content in the nav bar being justified to the right?

I'd like to see a navbar that is like

|[brand] [link1][link2]                [search][i1][i2]|

Where i1 and i2 are some icon such as a settings gear and profile icon.

@ocBruno
Copy link
Contributor Author

ocBruno commented Jan 24, 2020 •
edited
Loading

@ocBruno could we make the text of the nav links match the color of the brand and then be their current color when they are hovered over?

Plus, can the search bar remain to the left?

Can you provide a storybook example of content in the nav bar being justified to the right?

I'd like to see a navbar that is like

|[brand] [link1][link2]                [search][i1][i2]|

Where i1 and i2 are some icon such as a settings gear and profile icon.

Hey @jackcmeyer, thanks for the feedback once again.
I included the example and added the icons as requested and I also made className passable from component to it's parent element to improve customization.

As in maintaining the search bar to the left do you mean in the provided example or in one of the existing examples?

I'm not sure how we would go about adding the color according to the brand considering there could be some color or contrast conflicts. I think a good alternative is using the dark variant and letting the user customize any custom colors using css.

Also was wondering if we should make user-select: none; default to the nav item components.

@jackcmeyer
Copy link
Member

As in maintaining the search bar to the left do you mean in the provided example or in one of the existing examples?

@ocBruno Sorry, I meant the search bar to the right ☹️. I shouldn't look at Pull Requests late in the evening! The most recent example that you provided with icons looks perfect!

@ocBruno
refactor(navbar): refactor and modify tests and other layout changes
140d259 
change test naming convention, suggest removal of 2 tests and refactor one into two. Add margins to
navbar stories and throw search box to the right

fix #228
@ocBruno
Merge branch 'feat/support-search-position' of https://github.com/ocB…
04f74af 
…runo/components into feat/support-search-position
@ocBruno ocBruno changed the title WIP: feat(navbar): support navbar items horizontal position w array order feat(navbar): support navbar items horizontal position w array order Jan 25, 2020
@matteovivona matteovivona removed their request for review January 25, 2020 17:38
@ocBruno ocBruno changed the title feat(navbar): support navbar items horizontal position w array order feat(navbar): refactor navbar to support horizontal nav item placement and more Jan 25, 2020
jackcmeyer
jackcmeyer suggested changes Jan 26, 2020
View reviewed changes
scss/_variables.scss Outdated Show resolved Hide resolved
src/components/Navbar/Navbar.tsx Outdated Show resolved Hide resolved
@fox1t fox1t removed the request for review from irvelervel January 29, 2020 17:43
@matteovivona matteovivona self-assigned this Jan 30, 2020
@matteovivona matteovivona merged commit 4a5e59a into HospitalRun:master Jan 30, 2020
ghost pushed a commit that referenced this pull request Jan 30, 2020
@semantic-release-bot
chore(release): 0.32.0 [skip ci]
4f1436c 
# [0.32.0](v0.31.0...v0.32.0) (2020-01-30)

### Bug Fixes

* **navbar:** fix tests, refactor brand component to icon and header, ([91ce73e](91ce73e)), closes [#228](#228)
* **navbar:** remove className boilerplates ([1042a66](1042a66)), closes [#228](#228)
* **navbar:** remove test story ([c569ce6](c569ce6)), closes [#228](#228)
* **navbar:** repeated key warning in navbar items ([a52cff7](a52cff7)), closes [#228](#228)
* **navbar:** suggest new default and dark theme color, improve examples ([8ebbcf0](8ebbcf0)), closes [#228](#228)

### Features

* **navbar:** add example with icons and support className prop ([4784520](4784520)), closes [#228](#228)
* **navbar:** refactor navbar tests ([717a9fc](717a9fc)), closes [#225](#225)
* **navbar:** support classname for all components and justify example ([45faf19](45faf19)), closes [#228](#228)
* **navbar:** support navbar items horizontal position w array order ([f80382f](f80382f)), closes [#225](#225)
@ghost
Copy link

ghost commented Jan 30, 2020

🎉 This PR is included in version 0.32.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@ghost ghost added the released label Jan 30, 2020
@matteovivona matteovivona mentioned this pull request Feb 2, 2020
Closed
@ocBruno ocBruno deleted the feat/support-search-position branch December 22, 2020 16:47
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@jackcmeyer jackcmeyer jackcmeyer approved these changes

@fox1t fox1t fox1t approved these changes

Assignees

@matteovivona matteovivona

Labels
enhancement New feature or request released
Projects
None yet
Milestone
v1.0.0
Development

Successfully merging this pull request may close these issues.

Navbar should support adding elements to the right of the search box.
4 participants
@ocBruno @jackcmeyer @fox1t @matteovivona