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

Make Navbar mobile friendly #2117

Closed
jackcmeyer opened this issue Jan 17, 2020 · 21 comments · Fixed by #2118
Closed

Make Navbar mobile friendly #2117

jackcmeyer opened this issue Jan 17, 2020 · 21 comments · Fixed by #2118
Labels
🐛bug issue/pull request that documents/fixes a bug

Comments

@jackcmeyer
Copy link
Member

🐛 Bug Report

The Navbar is not mobile friendly.

To Reproduce

Render the Navbar on any mobile device

image

Expected behavior

The Navbar options should be collapsed into a hamburger button. One the hamburger button is clicked, it should have a dropdown with all of the options that are a part of the navbar.

@ocBruno
Copy link
Contributor

ocBruno commented Jan 17, 2020

On mobile the navbar now falls into a hamburger and is laid out vertically when opened, please spot me on any mistakes or non-ideal design patterns and considerations are always welcome!

@jackcmeyer
Copy link
Member Author

jackcmeyer commented Jan 17, 2020

@ocBruno please open a pull request so that we can review your changes.

If you’re not ready for it to be merged, but you want feedback, you can open a pull request and preface the title with ‘WIP’.

@ocBruno
Copy link
Contributor

ocBruno commented Jan 17, 2020

@ocBruno please open a pull request so that we can review your changes.

If you’re not ready for it to be merged, but you want feedback, you can open a pull request and preface the title with ‘WIP’.

Ah ok, will do! Thanks!

@andreykeycee
Copy link

Is that issue still open for work on it? I can make it

@fox1t
Copy link
Member

fox1t commented Feb 2, 2020

Hi @andreykeycee, yes it is still open. You can read some considerations about this here: HospitalRun/components#227

Do you want to give a shot yourself to this?

@andreykeycee
Copy link

@fox1t yeah, I want to, you can set me as assignee

@fox1t
Copy link
Member

fox1t commented Feb 2, 2020

@ocBruno What do you think? I am asking you because I know you were working on this. Are you still working on it or do you have any advices for @andreykeycee?

@ocBruno
Copy link
Contributor

ocBruno commented Feb 2, 2020

Good day @andreykeycee @fox1t !

Feel free to pick this one up Andrey I had put it on hold considering I was facing some difficulties. You are welcome to get my pull request and use it however needed as I almost had this one finished, here are a couple considerations I have from working on this.

  • I had added hamburger menu and made the navitems collapse accordingly, where I was having difficulties was testing the navbar styles.

  • I was using classes and thought it might be the problem so I refactored the classes to inline styles thinking I would be able to test with jest but was getting undefined in the style properties.

Thanks for the help and take care!

@andreykeycee
Copy link

@ocBruno, thank you for the detailed response! I will take a look at your PR.

@andreykeycee
Copy link

Hey guys, I just inspected those components, and how you work on it, and I wondering how carefully you're testing these, I like it! =) I have a couple of questions about it.

  • Is there any checklist, what exactly I should cover with tests?
  • Can I use Lodash to solve problems (I recognize you don't have it in dependencies)?

@ocBruno
Copy link
Contributor

ocBruno commented Feb 3, 2020

Hey guys, I just inspected those components, and how you work on it, and I wondering how carefully you're testing these, I like it! =) I have a couple of questions about it.

  • Is there any checklist, what exactly I should cover with tests?
  • Can I use Lodash to solve problems (I recognize you don't have it in dependencies)?

Hey @andreykeycee !

  • I think the best practice is cover the changes you're working on, so if the navbar collapses on mobile you have a test to make sure it hides the desktop navbar and displays the hamburger menu.

  • Also raising coverage by including any missing tests is always great!

  • I think depending on the use cases it might even be a good idea to inclue lodash to the dev dependencies but I'm not sure so best to double check with others.

Hey @jackcmeyer @fox1t!

  • If there are some good use cases for lodash should we include it in dev dependencies and use it or find work arounds?

@andreykeycee
Copy link

Hey, guys, I fix the problem itself, now navbar acts responsive, and also you can hide items on mobiles, if you want, but seems like I face the same issue, as @ocBruno with testing, and couldn't find any ways to check CSS via Enzyme, So I completely unavailable to check is components hides on mobiles.
Maybe anyone knows how to do that?

@ocBruno
Copy link
Contributor

ocBruno commented Feb 9, 2020

Hey, guys, I fix the problem itself, now navbar acts responsive, and also you can hide items on mobiles, if you want, but seems like I face the same issue, as @ocBruno with testing, and couldn't find any ways to check CSS via Enzyme, So I completely unavailable to check is components hides on mobiles.
Maybe anyone knows how to do that?

I was almost resorting to to using classes because if not mistaken I could find them with Enzyme but not sure if that is a good pattern/idea. If someone with more Enzyme/React testing knowledge could chime in !

@jackcmeyer
Copy link
Member Author

jackcmeyer commented Feb 9, 2020

There are a few different ways you can go about it.

The find function takes CSS selectors so you can find an element or component by css class.

Or, if you have already found the component or element you can do component.prop(className) to get the classes of the component.

Some links about testing screen size

https://stackoverflow.com/questions/46221210/jest-enzyme-how-to-test-at-different-viewports/51997030

https://airbnb.io/enzyme/docs/guides/jsdom.html

@ocBruno
Copy link
Contributor

ocBruno commented Feb 9, 2020

There are a few different ways you can go about it.

The find function takes CSS selectors so you can find an element or component by css class.

Or, if you have already found the component or element you can do component.prop(className) to get the classes of the component.

Some links about testing screen size

https://stackoverflow.com/questions/46221210/jest-enzyme-how-to-test-at-different-viewports/51997030

https://airbnb.io/enzyme/docs/guides/jsdom.html

Hey @andreykeycee!

@jackcmeyer left some good insights here, I believe if you still have trouble encountering with css rules and find, use classses for styling and try locating them by className!

I had closed my previous pull request after making all styles inline thinking I would be able to locate them with enzyme but debugging the wrappers always returned style={undefined}. As jack mentioned I think styling with bootstrap classes will allow you locate the component and use component.prop(className) !

@andreykeycee
Copy link

@jackcmeyer, @ocBruno thank you very much guys, I will try to use it tomorrow. It will be very helpful to get to know how to unit test viewport via jest!

@ocBruno
Copy link
Contributor

ocBruno commented Feb 10, 2020

@andreykeycee I'm not sure why I wasn't able to before but I am testing inline styling in PR HospitalRun/components#271 with the following code samples. Not sure if I was targeting the correct wrapper with props() but console.log(wrapper.debug()) should help you solve the issue!
I still haven't figured out how to located compiled styles which would be interesting but am not sure is necessary..

it('renders Text Input with custom style', () => {
  const wrapper = mount(<TextInput style={{ background: 'red' }} />)
  expect(wrapper.props().style).toMatchObject({ background: 'red' })
})

@andreykeycee
Copy link

andreykeycee commented Feb 12, 2020

The problem is there are not any changes in class names while resizing the screen. Bootstrap only changes the styles in media queries and it seems like I'm not allowed to read the component's not-inline CSS.
component.prop(className) works only if I passed some special class to component's props, but I can't pass any class, which will depend on the viewport. Also, I can't verify any nodes on existing, because bootstrap's "display: none" does not remove node from the Enzyme's virtual DOM.
I already changing viewport via
https://stackoverflow.com/questions/46221210/jest-enzyme-how-to-test-at-different-viewports/51997030
that example, but to be honest still not sure is that works properly.

@ocBruno
Copy link
Contributor

ocBruno commented Feb 12, 2020

If anyone has more experience with jest and can shed some light on properly testing dom style properties it would be much appreciated! This issue held me and @andreykeycee back from finishing up this contribution :/
If no answers appear soon I will post on jest repo asking for some assistance!

@ghost ghost self-assigned this Jun 4, 2020
@ghost ghost transferred this issue from HospitalRun/components Jun 4, 2020
@issue-label-bot
Copy link

Issue-Label Bot is automatically applying the label feature_request to this issue, with a confidence of 0.54. Please mark this comment with 👍 or 👎 to give our bot feedback!

Links: app homepage, dashboard and code for this bot.

@ghost
Copy link

ghost commented Jun 5, 2020

Hi, PR is ready for your review: #2118 Thank you!

@matteovivona matteovivona added 🐛bug issue/pull request that documents/fixes a bug and removed feature_request labels Jun 5, 2020
# for free to subscribe to this conversation on GitHub. Already have an account? #.
Labels
🐛bug issue/pull request that documents/fixes a bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants