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

Add icon-color prop to tip boxes #1284

Merged
merged 8 commits into from
Jul 24, 2020
Merged

Add icon-color prop to tip boxes #1284

merged 8 commits into from
Jul 24, 2020

Conversation

vig42
Copy link
Contributor

@vig42 vig42 commented Jul 24, 2020

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

• [ ] Documentation update
• [ ] Bug fix
• [ ] New feature
• [X] Enhancement to an existing feature
• [ ] Other, please explain:

Resolves #1271

What changes did you make? (Give an overview)
Added icon-color prop to tip boxes. This allows users to change the icon color independent of text color in tip boxes.

Some examples:

<box icon=":fas-times:" color = "grey" header = "Header" icon-color="blue">
    Lorem ipsum dolor sit amet, consectetur adipiscing elit, sed do eiusmod tempor incididunt ut labore et dolore magna aliqua. Ut enim ad minim veniam, quis nostrud exercitation ullamco laboris nisi ut aliquip ex ea commodo consequat.
</box>

image

<box seamless icon=":fas-plus:" icon-color="red">
    Lorem ipsum dolor sit amet, consectetur adipiscing elit, sed do eiusmod tempor incididunt ut labore et dolore magna aliqua. Ut enim ad minim veniam, quis nostrud exercitation ullamco laboris nisi ut aliquip ex ea commodo consequat.
</box>

image

<box type="info" icon-color="pink">
    Lorem ipsum dolor sit amet, consectetur adipiscing elit, sed do eiusmod tempor incididunt ut labore et dolore magna aliqua. Ut enim ad minim veniam, quis nostrud exercitation ullamco laboris nisi ut aliquip ex ea commodo consequat.
</box>

image

Note that you will need to run npm run build:components in order to see the changes, as mentioned here.

Is there anything you'd like reviewers to focus on?
Additions were made to packages/vue-components/src/TipBox.vue

Testing instructions:
npm run test should pass

Proposed commit message: (wrap lines at 72 characters)
Add icon-color prop to tip boxes

Copy link
Contributor

@ang-zeyu ang-zeyu left a comment

Choose a reason for hiding this comment

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

This looks great! Thanks! @vig42

@ang-zeyu ang-zeyu added this to the v2.15.0 milestone Jul 24, 2020
@ang-zeyu
Copy link
Contributor

On the branch naming, do make PRs from separate branches though, otherwise you might have problems resolving with the upstream later.
It's also slightly clearer for others looking through the history (for PRs with multiple commits to be merged), but it shouldn't be an issue here since this is small enough to be squashed :)

@damithc
Copy link
Contributor

damithc commented Jul 24, 2020

Good work @vig42
A question: is it possible to use bootstrap color names here e.g., danger info etc.?

@vig42
Copy link
Contributor Author

vig42 commented Jul 24, 2020

@ang-zeyu Noted, thank you!

@damithc Not as it stands right now. The icon-color prop only works with CSS legal color values, same as the color prop.

@ang-zeyu ang-zeyu merged commit 239c92f into MarkBind:master Jul 24, 2020
@damithc
Copy link
Contributor

damithc commented Jul 24, 2020

@damithc Not as it stands right now. The icon-color prop only works with CSS legal color values, same as the color prop.

Noted @vig42 What are your thoughts on supporting that feature?

@vig42
Copy link
Contributor Author

vig42 commented Jul 24, 2020

@damithc Not as it stands right now. The icon-color prop only works with CSS legal color values, same as the color prop.

Noted @vig42 What are your thoughts on supporting that feature?

We could implement this by dynamically applying the corresponding bootstrap class (info, warning etc.) to the icon wrapper div, to override the overall bootstrap class. We would have to have a check in place if the seamless or light styles are used, otherwise both the classes would be applied to the icon wrapper div. We could similarly do this to the content wrapper if we want the same functionality for the color property.

I guess the use case would be for someone who wants to combine the background or text color of 1 bootstrap style with the icon color of another.

@damithc
Copy link
Contributor

damithc commented Jul 24, 2020

We could implement this by dynamically applying the corresponding bootstrap class (info, warning etc.) to the icon wrapper div, to override the overall bootstrap class. We would have to have a check in place if the seamless or light styles are used, otherwise both the classes would be applied to the icon wrapper div. We could similarly do this to the content wrapper if we want the same functionality for the color property.

I guess the use case would be for someone who wants to combine the background or text color of 1 bootstrap style with the icon color of another.

Doesn't sound like worth the effort. Perhaps we can provide the color codes as built-in global variables e.g., icon-color="{{ color_primary }}"? @ang-zeyu what do you think? in terms for development and performance costs?

@ang-zeyu
Copy link
Contributor

Doesn't sound like worth the effort. Perhaps we can provide the color codes as built-in global variables e.g., icon-color="{{ color_primary }}"? @ang-zeyu what do you think? in terms for development and performance costs?

Do-able as @vig42 mentioned, but it's a little strange for a color prop to refer to class names to me. (you might also have authors wondering why they can't do icon-color="my-class", which is then difficult)

Color codes as built in variables would be certainly nice, but a lot of work has to be done before that similar to what's needed for #903. That and color codes are definitely worth the time / code to me though

@damithc
Copy link
Contributor

damithc commented Jul 25, 2020

Do-able as @vig42 mentioned, but it's a little strange for a color prop to refer to class names to me. (you might also have authors wondering why they can't do icon-color="my-class", which is then difficult)

I agree with that. Let's not go there.

Color codes as built in variables would be certainly nice, but a lot of work has to be done before that similar to what's needed for #903. That and color codes are definitely worth the time / code to me though

The objective is to let users use exact bootstrap colors where MarkBind syntax doesn't accept bootstrap color names (e.g., icon-color, but we have a few other places). Note that these built-ins are not meant to be changed by the user. It's just a way to accessing the equivalent hex code for bootstrap defaults. Otherwise, users will have to look up the values somewhere (I don't know even if there is a place to look it up) and use them as magic numbers in the code. So, instead of icon-color="#0275d8", they can use icon-color="{{ color_primary }}" without even knowing the hex code.

I think this is just a matter of adding a bunch of global named constants (not correct to say variables, although we can keep calling them builtin variables for simplicity). We already have some global variables https://markbind.org/userGuide/reusingContents.html#built-in-global-variables that vary the value dynamically e.g., timestamp Shouldn't global constants be even easier to add as they are static?

@ang-zeyu
Copy link
Contributor

The objective is to let users use exact bootstrap colors where MarkBind syntax doesn't accept bootstrap color names (e.g., icon-color, but we have a few other places). Note that these built-ins are not meant to be changed by the user. It's just a way to accessing the equivalent hex code for bootstrap defaults. Otherwise, users will have to look up the values somewhere (I don't know even if there is a place to look it up) and use them as magic numbers in the code. So, instead of icon-color="#0275d8", they can use icon-color="{{ color_primary }}" without even knowing the hex code.

I think this is just a matter of adding a bunch of global named constants (not correct to say variables, although we can keep calling them builtin variables for simplicity). We already have some global variables https://markbind.org/userGuide/reusingContents.html#built-in-global-variables that vary the value dynamically e.g., timestamp Shouldn't global constants be even easier to add as they are static?

unfortunately, that would be the case if we didn't have support for bootstrap themes, but even if we didn't, I don't think it's good practice to hard code these values on the development side; it's a lot of maintainence.

internally, bootstrap declares it's color palette with sass variables, something like this (example from a bootswatch theme):

$primary:       $blue !default;
$secondary:     $gray-800 !default;
$success:       $green !default;
$info:          $cyan !default;
$warning:       $yellow !default;
$danger:        $red !default;
$light:         $gray-100 !default;
$dark:          $gray-800 !default;

it may be much more extensible to have a build process automatically source the color codes from these source files, and output them as variables. This is also in line with #903 which likely requires a similar sourcing process (but from the author side).

lots of things need to be done before then though, this and #903 would probably be a v3.0 feature

@damithc
Copy link
Contributor

damithc commented Jul 25, 2020

it may be much more extensible to have a build process automatically source the color codes from these source files, and output them as variables. This is also in line with #903 which likely requires a similar sourcing process (but from the author side).

lots of things need to be done before then though, this and #903 would probably be a v3.0 feature

Understood. In the meantime, where can I find the hex values for those 8 colors?

@ang-zeyu
Copy link
Contributor

I would go here https://getbootstrap.com/docs/4.0/utilities/colors/ and use inspect element to get the colors. It's a little cumbersome but it's only do-once and wysiwyg.

Untitled

@damithc
Copy link
Contributor

damithc commented Jul 25, 2020

I would go here https://getbootstrap.com/docs/4.0/utilities/colors/ and use inspect element to get the colors. It's a little cumbersome but it's only do-once and wysiwyg.

Nice. Thx @ang-zeyu

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add icon-color prop to tip boxes
3 participants