Skip to content
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

Improve color theme selection for navbars #797

Merged
merged 5 commits into from
Apr 1, 2019
Merged

Conversation

luyangkenneth
Copy link
Contributor

@luyangkenneth luyangkenneth commented Mar 31, 2019

What is the purpose of this pull request? (put "X" next to an item, remove the rest)

• [X] Enhancement to an existing feature

Fixes #386

What is the rationale for this request?
This PR allows users to customize their navbar colors. There is now an option to choose from 3 color schemes that are provided by each bootstrap/bootswatch theme.

There's also an option to specify type="none", for users who wish to inject their own CSS classes using our existing add-class feature.

In short, <navbar> now has type="primary" | "dark" | "light" | "none", instead of type="inverse" | "default".

What changes did you make? (Give an overview)

Testing instructions:

  1. Look at https://deploy-preview-797--markbind-master.netlify.com/userguide/usingcomponents#navbars for the code and the corresponding output.
  2. Change the navbar type to none or some gibberish value (this will default to primary), observe that the rendered navbar is styled correctly.

@luyangkenneth luyangkenneth changed the title Allow navbar theming Improve color theme selection for navbars Mar 31, 2019
@luyangkenneth
Copy link
Contributor Author

Using the default bootstrap theme

Screenshot 2019-03-31 at 11 59 27 PM

Using the bootswatch-united theme

Screenshot 2019-04-01 at 12 03 14 AM

@luyangkenneth luyangkenneth force-pushed the allow-navbar-theming branch 2 times, most recently from 30c8c6f to f0b3bb8 Compare April 1, 2019 03:50
Copy link
Member

@yamgent yamgent left a comment

Choose a reason for hiding this comment

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

Propose a merge commit message. Also your associated vue-strap PR have been merged, use the latest master branch to replace vue-strap.min.js?

Btw a side note, actually PR authors shouldn't be the ones re-generating vue-strap.min.js, but right now we don't have a proper packaging system for vue-strap that is specialized for MarkBind, so I can understand the need of re-generating vue-strap.min.js by yourself. In the future, I foresee such updates to be merely just a change in the version number (i.e. modify package.json to up the version number of vue-strap).

@luyangkenneth luyangkenneth force-pushed the allow-navbar-theming branch from f0b3bb8 to 0ded524 Compare April 1, 2019 08:23
@luyangkenneth
Copy link
Contributor Author

Just did a force push 👍

Here's my proposed commit message:

Improve color theme selection for navbars (#797)

* Modify the Navbar.vue component to take in a `type` of
  `primary | dark | light | none` so as to allow users to select
  navbar colors based on each Bootstrap/Bootswatch theme.

* Update all <navbar> instances in docs and tests

@luyangkenneth luyangkenneth added the breakingChange 💥 Feature will behave significantly different, or is made obsolete label Apr 1, 2019
@yamgent yamgent merged commit 8e0f2a8 into master Apr 1, 2019
@yamgent yamgent added this to the v2.0.1 milestone Apr 1, 2019
@luyangkenneth luyangkenneth deleted the allow-navbar-theming branch April 1, 2019 16:12
@damithc
Copy link
Contributor

damithc commented Apr 2, 2019

Gave it a try. Nice 👍
Also discovered that add-class works even if the type is not none. e.g., type="primary" add-class="bg-danger" would give you a nice red navbar.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breakingChange 💥 Feature will behave significantly different, or is made obsolete
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants