New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Custom navbar #118

Merged
merged 16 commits into from Jan 6, 2019

Conversation

Projects
None yet
2 participants
@tcbegley
Copy link
Member

tcbegley commented Dec 28, 2018

This PR adds a CustomNavbar component giving users full control over the children, addressing #101

@tcbegley tcbegley force-pushed the custom-navbar branch from a6341e5 to 98adc2b Dec 28, 2018

@tcbegley tcbegley force-pushed the custom-navbar branch from d7f1583 to 8f8aaf5 Dec 30, 2018

@tcbegley tcbegley force-pushed the custom-navbar branch from 1f82271 to 39b289b Dec 30, 2018

@tcbegley tcbegley changed the title [WIP] Custom navbar Custom navbar Dec 30, 2018

@tcbegley

This comment has been minimized.

Copy link
Member

tcbegley commented Dec 30, 2018

This is now ready for testing and review. I have

  • Corrected NavbarBrand import in index.js and set it to use a span element if href isn't set.
  • Added n_clicks and n_clicks_timestamp to NavbarToggler to allow it to be used in callbacks.
  • Added several examples to examples/advanced-component-usage/CustomNavbar.py
  • Added Navbar and CustomNavbar to the documentation, thereby resolving #75

I've made prerelease 0.2.7rc3 with these changes, you can try out the examples if you first run pip install dash-bootstrap-components==0.2.7rc3. The examples app looks something like this

image

It's also worth trying them out with the different Bootswatch themes and on smaller screens, I got some pretty fun results.

screencapture-small

@pbugnion

This comment has been minimized.

Copy link
Member

pbugnion commented Jan 3, 2019

This is great, thanks!

Should we:

  1. put a link to the advanced-components-usage/CustomNavbar.py example in the markdown used to document navbars? Otherwise, the examples are not very discoverable, which is a shame because they're great.
  2. consider whether CustomNavbar is really the best name? We could imagine renaming Navbar -> SimpleNavbar (or NavbarSimple for discoverability) and have CustomNavbar as Navbar, since it's closer to the original Bootstrap navbar. Alternatively, we could rename CustomNavbar to RawNavbar (or NavbarRaw) or BasicNavbar?
@tcbegley

This comment has been minimized.

Copy link
Member

tcbegley commented Jan 3, 2019

Thanks for reviewing!

  1. Yes, good idea, I'll add that.
  2. Agreed, I think that reserving Navbar for the fully customisable option to more closely shadow Bootstrap is probably preferable. The main reason I opted for CustomNavbar was to preserve backwards compatibility. If we decide to break that, what's the best way we can warn users who have already used Navbar in their code that the usage will change if they upgrade?
Show resolved Hide resolved src/components/nav/Navbar.js Outdated
@pbugnion

This comment has been minimized.

Copy link
Member

pbugnion commented Jan 4, 2019

The main reason I opted for CustomNavbar was to preserve backwards compatibility.

We are still at a stage where we can break backwards compatibility without too many consequences, I think.

what's the best way we can warn users who have already used Navbar in their code that the usage will change if they upgrade?

I think that, if dbc was more mature, the correct way would be to introduce a deprecation cycle like:

  1. rename Navbar to CustomNavbar. Have Navbar just proxy to CustomNavbar, but raising a deprecation warning when you try to use it.
  2. wait some length of time proportional to how mature the project is.
  3. replace existing Navbar with the new code and bump the version number.

This gives people a chance to migrate their code. In the interest of moving forward, a more pragmatic solution might be to do a cursory check of libraries that we think use dash-bootstrap-components to see if they use Navbar. We can then just raise an issue with them.

AFAICT, this is what I've found on github so far:

  • valveca-ai/dash-dev
  • villoro/expensor_personal

Obviously this doesn't cover private code repositories, code that's not on github or not version controlled etc., but sometimes you have to cut your losses.

Relatedly, this might be a good time to introduce a changelog into the documentation.

@tcbegley

This comment has been minimized.

Copy link
Member

tcbegley commented Jan 4, 2019

Ok, this time I think this is actually ready for review, sorry for jumping the gun just now. I've refactored the tests, demo, examples and docs and checked that all mentions of Navbar refer to the new component.

@pbugnion

This comment has been minimized.

Copy link
Member

pbugnion commented Jan 6, 2019

I think this is great -- made one comment inline but, apart from that, we should merge this.

@tcbegley tcbegley merged commit 5aa6c04 into master Jan 6, 2019

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
@tcbegley

This comment has been minimized.

Copy link
Member

tcbegley commented Jan 6, 2019

@pbugnion given the breaking change to the Navbar component shall I release this as 0.3.0 rather than 0.2.9?

@tcbegley tcbegley referenced this pull request Jan 6, 2019

Closed

Document Navbar component #75

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment