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

Allow custom colors for top nav bar #386

Closed
damithc opened this issue Jul 30, 2018 · 30 comments
Closed

Allow custom colors for top nav bar #386

damithc opened this issue Jul 30, 2018 · 30 comments
Assignees
Labels
a-Theming breakingChange 💥 Feature will behave significantly different, or is made obsolete c.Enhancement f-TopNav

Comments

@damithc
Copy link
Contributor

damithc commented Jul 30, 2018

v1.1.10

Current: there are only two choices for top nav bar color: black and grey

Suggestion: Allow custom colors

Justification: To create a lightweight theme effect on sites. Especially useful when instructors work with several variations of the same site and want an easy way to distinguish between them.

@acjh
Copy link
Contributor

acjh commented Jul 30, 2018

I feel that this one-off customisation should be done with custom styling in the site's own CSS file, rather than supported by MarkBind or MarkBind/vue-strap.

There is little value that MarkBind can add, as compared to a request like #69 (Support custom boxes).

@damithc
Copy link
Contributor Author

damithc commented Jul 30, 2018

I feel that this one-off customisation should be done with custom styling in the site's own CSS file, rather than supported by MarkBind or MarkBind/vue-strap.

Fair point. The counter-point is that every website would want to brand itself. Although it is once-per-website, it might be used by most markbind sites.

If we leave it as DIY, we might still want to give guidance in documentation e.g., which class to override? Note that our target users are instructors. Until we have a WYSIWYG editor, we are forced to assume the target users have (or can learn) some basic programming skills but we cannot assume web programming expertise.

It also depend on the cost of the feature too.

For the time being, for my own sites, I'm OK as long as I'm told how to override the color :-)

@acjh
Copy link
Contributor

acjh commented Jul 30, 2018

.navbar {
    background-colour: green !important;
}

.navbar * {
    color: black !important;
}

@damithc
Copy link
Contributor Author

damithc commented Jul 30, 2018

Thanks @acjh Works after fixing typo background-colourbackground-color
Is it possible to give different colors to main text and drop-down text?

@yamgent
Copy link
Member

yamgent commented Jul 30, 2018

Dropdown:

.dropdown-item {
  color: brown !important
}

main text

Main text as in the website page content itself?

@damithc
Copy link
Contributor Author

damithc commented Jul 30, 2018

Thanks @yamgent

Main text as in the website page content itself?

No. I meant text in the main nav bar vs text in the dropdown items. Your solution is probably what I was looking for. Will try it out.

@acjh
Copy link
Contributor

acjh commented Jul 30, 2018

If you don't want to affect other dropdowns:

.navbar .dropdown-item {
  color: brown !important
}

@damithc
Copy link
Contributor Author

damithc commented Jul 30, 2018

I've tried using these on the website-base. While they work, it is quite a lot of css code to theme the whole navbar to a different color, including text and icons to match, not forgetting the hover over colors, boarders, shadows etc. I didn't manage to make it as good-looking as the default theme. I guess theming is harder than it appears at first :-p

@acjh
Copy link
Contributor

acjh commented Jul 31, 2018

Indeed. But there is little value that MarkBind can add.

MarkBind should go for pleasant styling, but theming does not need further support in the codebase.
#363 (Support adding custom classes to any vue-strap component) was a step in the right direction.

Theming can be a topic to be considered when doing #371 (Give examples for adding custom classes).

@damithc
Copy link
Contributor Author

damithc commented Jul 31, 2018

I guess what I was wishing for here is to be able to specify a base color and the whole nav bar to change to match the color, i.e., almost like inbuilt themes for nav bar where I specify a color instead of a theme name. I guess that requires a lot of work behind the scene.

@acjh
Copy link
Contributor

acjh commented Jul 31, 2018

If you mean like Bootstrap 4's navbar-dark bg-dark classes, that can be done purely in CSS.
Not a lot of work, but custom theming has not a lot of value either (unlike default/dark themes).

We can create an example theming file though.

@damithc
Copy link
Contributor Author

damithc commented Jul 31, 2018

If you mean like Bootstrap 4's navbar-dark bg-dark classes, that can be done purely in CSS.

I guess. More specifically, I wish I could specify just a base color (maroon?) and get a navbar like the one below. i.e., MarkBind figures out all the matching color variations for text, outlines, shadows, hover over etc.

image

Not a lot of work, but custom theming has not a lot of value either (unlike default/dark themes).

I agree there is not much real value. But there is some perceived value because people don't want their site to look like everyone else's.

In any case, this is not an urgent thing. At the moment we are not pushing to get more users. :-p

@acjh
Copy link
Contributor

acjh commented Jul 31, 2018

White text is what navbar-dark is for. Changing the colours in the input though...

@damithc
Copy link
Contributor Author

damithc commented Jan 26, 2019

Just noticed that Bootstrap already supports a bunch of color schemes for nav bars. Can be port that over to MarkBind?

@acjh
Copy link
Contributor

acjh commented Jan 26, 2019

All 3 of those already work:

  • navbar-dark bg-dark
  • navbar-dark bg-primary
  • navbar-light

@damithc
Copy link
Contributor Author

damithc commented Jan 27, 2019

All 3 of those already work:

  • navbar-dark bg-dark
  • navbar-dark bg-primary
  • navbar-light

Like this <navbar placement="top" class="navbar-dark bg-primary">?

@yamgent
Copy link
Member

yamgent commented Jan 27, 2019

<navbar placement="top" class="navbar-dark bg-primary">

How about we support more types? E.g. <navbar placement="top" type="primary">

navbar-dark and navbar-light controls the foreground text color. If the user wants to fine-tune these, support an additional third option? <navbar placement="top" type="primary" fgcolor="light">

@acjh
Copy link
Contributor

acjh commented Jan 27, 2019

Aren't users already able to add classes?

@yamgent
Copy link
Member

yamgent commented Jan 28, 2019

Aren't users already able to add classes?

That is true, users can just directly apply classes, there is no need to support additional attributes.

@damithc
Copy link
Contributor Author

damithc commented Jan 28, 2019

That is true, users can just directly apply classes, there is no need to support additional attributes.

So, like this? <navbar placement="top" type="inverse" add-class="navbar-dark bg-primary">
Doesn't seem to work either.

@yamgent
Copy link
Member

yamgent commented Jan 28, 2019

So, like this? <navbar placement="top" type="inverse" add-class="navbar-dark bg-primary">
Doesn't seem to work either.

It doesn't work because type itself adds a bunch of navbar- and bg- classes, so if authors add additional ones, there will be conflicts. This applies even if type is not specified (the default also adds some classes).

While we already add author's custom classes at the end of the class list of the navbar, apparently the order in the class list doesn't even matter. It is only the order of the classes declared inside the css that matters.

So going forward, are we sticking with fixing the add-class behaviour so that if it contains navbar-* and bg-*, type will automatically "obey" them and not add classes themselves? Or should we consider expanding the syntax?

@acjh
Copy link
Contributor

acjh commented Jan 28, 2019

I suggest we default type to 'none' as done for <tip-box>, rather than interpreting the classes.

We could expand the syntax to support common styles as done in MarkBind/vue-strap#26, but I believe it's counter-productive as navbar will probably be themed using classes, which we already support and should encourage authors to use.

@damithc
Copy link
Contributor Author

damithc commented Jan 28, 2019

So going forward, are we sticking with fixing the add-class behaviour so that if it contains navbar-* and bg-*, type will automatically "obey" them and not add classes themselves? Or should we consider expanding the syntax?

add-class overrides default class is a good policy to have in general, right? I think users will expect that from add-class, unless we say otherwise.

@yamgent
Copy link
Member

yamgent commented Jan 28, 2019

add-class overrides default class is a good policy to have in general, right? I think users will expect that from add-class, unless we say otherwise.

Yes, but if the classes all resides in the same .css file, we do not have control over how the override happens.

We could follow @acjh's suggestion on allowing a type="none", so that the user is able to specify the navbar-* and bg-* that they want to use via add-class. The documentation will need to specify that using both a non-none type and Bootstrap navbar classes via add-class will cause unexpected behaviours, and should not be used together.

@damithc
Copy link
Contributor Author

damithc commented Jan 28, 2019

We could follow @acjh's suggestion on allowing a type="none", so that the user is able to specify the navbar-* and bg-* that they want to use via add-class. The documentation will need to specify that using both a non-none type and Bootstrap navbar classes via add-class will cause unexpected behaviours, and should not be used together.

Sounds good. 👍

@luyangkenneth
Copy link
Contributor

luyangkenneth commented Mar 26, 2019

(Following up on our discussion in #745 to enable all navbar colors within any given theme)

Can someone clarify my understanding that, for our navbar, type="inverse" | "default" is only for specifying its navbar-dark/navbar-light and bg-dark/bg-light attributes?

Now that we support bootswatch themes, the classes are always combined like this to control how the theme colors show up (doesn't matter whether the text color is actually dark or light):

  • navbar-dark bg-primary
  • navbar-dark bg-dark
  • navbar-light bg-light

So I'm wondering if we can go with type="primary" | "dark" | "light" to allow users to change the navbar color, given a particular theme. In that case, a user can just specify one single attribute in their <navbar>, and not need to use add-class.

@acjh
Copy link
Contributor

acjh commented Mar 26, 2019

Note that navbar-dark (i.e. light-coloured text) may not be suitable for all bg-primary colours.

@luyangkenneth
Copy link
Contributor

luyangkenneth commented Mar 26, 2019

Yep, the reason why I mention "doesn't matter whether the text color is actually dark or light" is because bootswatch themes take care of it.

For example, in https://bootswatch.com/darkly/ the "light" navbar is still a dark one - you can see the code by hovering over a navbar and clicking on the <> button at its top-right corner. (I'm giving a dark theme example because all bootswatch themes have light coloured text when the navbar is set to bg-primary)

@luyangkenneth
Copy link
Contributor

luyangkenneth commented Mar 28, 2019

I discussed offline with @yamgent about this. Basically the idea is to go ahead with an implementation that uses type="primary" | "dark" | "light" | "none" for the <navbar>.

This allows 2 options for the user:

  1. Easy color selection without messing with CSS classes. The user specifies type="primary" | "dark" | "light" to select one of the 3 available colors from the theme. This works for the default bootstrap theme as well. For this option, the navbar-* and bg-* classes will be automatically added.

  2. Customizable. The user specifies type="none" and make use of add-class to customize the <navbar> however they like.

@luyangkenneth luyangkenneth added the breakingChange 💥 Feature will behave significantly different, or is made obsolete label Mar 28, 2019
@luyangkenneth
Copy link
Contributor

luyangkenneth commented Mar 28, 2019

The navbar changes in #790 are blocking this, so I'll wait until that PR is merged before working on this.

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

No branches or pull requests

4 participants