TipBox.vue: Support different size icons #124

yash-chowdhary · 2020-01-22T05:48:53Z

closes MarkBind #986 (specifications of sizes can also be found there).

This is an enhancement to box icons.
Usage:

<box type="warning" icon=":fas-camera:" icon-size="2x">
    DUMMY TEXT
</box>

(Possible) Expected output(s):

icon-size="2x", single-para
icon-size="2x", multi-para
icon-size="2x", multi-para, no heading, not dismissible

yash-chowdhary · 2020-01-22T11:38:22Z

Ready for Review

acjh

Only the .icon-wrapper change appears to be necessary.
My thoughts on styling: MarkBind/markbind#386 (comment), MarkBind/markbind#386 (comment)

Suggested usage:

<box icon=":rocket:" add-class="icon-large">
    DUMMY TEXT
</box>

.icon-large i {
    font-size: 3em;
}

src/TipBox.vue

acjh · 2020-01-24T06:02:53Z

src/TipBox.vue

+        flex-direction: column;
+        justify-content: center;
+        align-content: center;
+        font-size: medium;


I believe we only need display and align-content.

I tried it without the flex-direction: column, and the justify-content: center. Here's what it looks like for large paragraphs -

Here's what it looks like without justify-content: center (but with flex-direction: column). While it does work for long paragraphs, single-line content is not properly aligned -

That appears to be because <slot></slot> is directly in contents, together with heading and dismiss-button.

I don't think it is appropriate to always centre the children in contents though, particularly the heading. Did we try extra-large (maybe extremely-large) with heading and/or dismissable?

Sorry for the late reply. I've been trying out different things to make this work. You're right, the heading and dismiss-button don't behave well when I center-align the contents. Center-aligning the contents is much harder than I thought it would be 😅

I've tried wrapping the <slot> in a <div> and <span>, but that doesn't work well. The next thing I tried was moving the heading and button outside the 'content's div and make it a 3 column container (one for the icon, one for text, one for heading and button). The problem here is that the heading messes with the text column if it is too long.
So, the latest thing I'm working on is separating out the heading into a div that would be placed on top of the container, effectively making it a 2-rowed container. The 2nd row will be a 3 column container, with the 3rd column only for the dismiss-button.

Are there any better ways to deal with this?

I would say not to vertically center-align the contents.

Alright 👍

src/TipBox.vue

yash-chowdhary · 2020-01-25T04:15:05Z

As mentioned above, usage:

<box type="warning" add-class="icon-large">
    DUMMY TEXT
</box>

Will open a PR in markbind to add the CSS icon-size classes to the assets.

damithc · 2020-01-25T04:44:39Z

As mentioned above, usage:

<box type="warning" add-class="icon-large">
    DUMMY TEXT
</box>

Oh? I thought it is going to be <box type="warning" icon-large>

yash-chowdhary · 2020-01-25T05:53:33Z

I thought @acjh wanted to normalize it to using add-class?

damithc · 2020-01-25T06:09:07Z

I thought @acjh wanted to normalize it to using add-class?

@acjh what do you think? <box type="warning" icon-large> is easier to the author but I don't mind using <box type="warning" add-class="icon-large"> if there a justification.

acjh · 2020-01-25T09:20:03Z

Will open a PR in markbind to add the CSS icon-size classes to the assets.

By the site's own CSS file, I meant like https://github.com/nus-cs2103/website-base/blob/master/css/main.css.

@acjh what do you think? <box type="warning" icon-large> is easier to the author but I don't mind using <box type="warning" add-class="icon-large"> if there a justification.

The styles are arbitrary (2em, 3em, extra-large). If we do this, then authors may expect MarkBind to add more styles like medium-large (2.5em), extra-extra-large (5em) and text sizes next. There is little value that MarkBind can add, especially since these can be achieved with plain CSS quite easily. MarkBind should go for pleasant styling, maybe with alternative defaults if reasonable, but we should draw the line between supporting the ability to do something (add-class and the other changes in this PR) and adding arbitrary styles.

damithc · 2020-01-25T11:32:45Z

Will open a PR in markbind to add the CSS icon-size classes to the assets.

By the site's own CSS file, I meant like https://github.com/nus-cs2103/website-base/blob/master/css/main.css.

@acjh what do you think? <box type="warning" icon-large> is easier to the author but I don't mind using <box type="warning" add-class="icon-large"> if there a justification.

The styles are arbitrary (2em, 3em, extra-large). If we do this, then authors may expect MarkBind to add more styles like medium-large (2.5em), extra-extra-large (5em) and text sizes next. There is little value that MarkBind can add, especially since these can be achieved with plain CSS quite easily. MarkBind should go for pleasant styling, maybe with alternative defaults if reasonable, but we should draw the line between supporting the ability to do something (add-class and the other changes in this PR) and adding arbitrary styles.

I agree with drawing the line somewhere. I'm also mindful that our target authors are expected to be able to use basic html-like syntax only whereas css editing is moving to web developer territory. As an author, I myself sometimes ask you guys for css hacks when I can't get MarkBind to do what I want. It's not something we can expect our target users to do (at least not the authors of education materials; might be OK for those writing project documentation if the project itself is a web project). So, it's good if we can minimize the need for css editing.

For example, the box icon size vary between different site generators (ours seem to be the smallest) but probably the 4 proposed variations can cover the full range. That way, our box can be configured to be close to most of the common box styles. If the author wants some odd-sized icon, only then they should have to resort to css editing.

Do you think it is OK to provide some built-in styles, like how Bootstrap does (e.g., text-danger text-info pr-1 pr-2 etc.)? That way, our users can kind of make their own styles without having to write css code themselves. That is, to have something like this in the UG:

You can vary the icon size by adding icon-medium, icon-large, icon-extralarge to a box via the add-class mechanism. If those sizes don't fit your needs, you can set it to your own size as follows:
Add a class to your site's custom css file.
Example:
.icon-custom i {
   font-size: 3em;
}
Then, ...

acjh · 2020-01-25T15:21:15Z

Do you think it is OK to provide some built-in styles, like how Bootstrap does (e.g., text-danger text-info pr-1 pr-2 etc.)?

I think that is not where MarkBind should be heading.

Some investigation shows that Font Awesome already does sizing:
https://fontawesome.com/how-to-use/on-the-web/styling/sizing-icons

fa-lg is 1.33333em — large is arbitrary.

How about:

<!-- Raw -->
<box type="warning" icon=":fas-camera:" icon-size="2x">
    DUMMY TEXT
</box>

<!-- Rendered -->
<div class="alert container alert-warning">
  <div class="icon-wrapper fa-2x">
    <span><i class="fas fa-camera"></i></span>
  </div>
  <div class="contents">
    DUMMY TEXT
  </div>
</div>

Note that fa-{iconSize} is added to icon-wrapper rather than the icon element i itself, unlike the Font Awesome examples, to simplify support for custom Markdown icon.

damithc · 2020-01-25T15:36:35Z

<box type="warning" icon=":fas-camera:" icon-size="2x"> sounds good.

yash-chowdhary · 2020-01-30T14:08:36Z

Usage:

<box type="warning" icon=":fas-camera:" icon-size="2x">
    DUMMY TEXT
</box>

(Possible) Expected output(s):

icon-size="2x", single-para
icon-size="2x", multi-para
icon-size="2x", multi-para, no heading, not dismissible

acjh

Thanks for your changes. We can clean up a little.

acjh · 2020-01-30T14:47:26Z

src/TipBox.vue

+        <div v-if="!isDefault && !iconSize" class="icon-wrapper">
+            <span v-html="iconType"></span>
+        </div>
+        <div v-if="!isDefault && iconSize" class="icon-wrapper" :class="`fa-${iconSize}`" >


Let's do as done with lightStyle:

<div v-if="!isDefault" class="icon-wrapper" :class="[iconStyle]">

src/TipBox.vue

acjh · 2020-02-01T02:33:53Z

src/TipBox.vue

@@ -1,6 +1,6 @@
 <template>
    <div class="alert container" :class="[boxStyle, addClass, lightStyle]" :style="customStyle">
-        <div v-if="!isDefault" class="icon-wrapper">
+        <div v-if="!isDefault" class="icon-wrapper" :class="[`fa-${iconSize}`]" >


Wouldn't this result in class="fa-null" if iconSize not specified?

acjh · 2020-02-01T16:33:20Z

src/TipBox.vue

@@ -1,6 +1,6 @@
 <template>
    <div class="alert container" :class="[boxStyle, addClass, lightStyle]" :style="customStyle">
-        <div v-if="!isDefault" class="icon-wrapper">
+        <div v-if="!isDefault" class="icon-wrapper" :class="`${iconSize ? `fa-${iconSize}`: ''}`" >


Let's do as done with lightStyle as suggested in #124 (comment):

<div v-if="!isDefault" class="icon-wrapper" :class="[iconStyle]">

acjh · 2020-02-02T14:45:30Z

src/TipBox.vue

@@ -1,6 +1,6 @@
 <template>
    <div class="alert container" :class="[boxStyle, addClass, lightStyle]" :style="customStyle">
-        <div v-if="!isDefault" class="icon-wrapper">
+        <div v-if="!isDefault" class="icon-wrapper" :class="[iconStyle]" >


Remove stray whitespace:

Suggested change

<div v-if="!isDefault" class="icon-wrapper" :class="[iconStyle]" >

<div v-if="!isDefault" class="icon-wrapper" :class="[iconStyle]">

acjh · 2020-02-02T14:50:15Z

src/TipBox.vue

      }
    }
  }
+  }


Misplaced closing brace.

Missing default return value.

iconStyle() { if (this.iconSize) { return `fa-${this.iconSize}`; + } + return ''; } } } - }

yamgent

Remember to update the docs in the MarkBind repo.

acjh reviewed Jan 22, 2020

View reviewed changes

src/TipBox.vue Outdated Show resolved Hide resolved

acjh reviewed Jan 24, 2020

View reviewed changes

This was referenced Jan 27, 2020

Add bootswatch theme specific tip box styles MarkBind/markbind#999

Closed

<box>: support seamless style MarkBind/markbind#987

Closed

acjh reviewed Jan 30, 2020

View reviewed changes

acjh reviewed Feb 1, 2020

View reviewed changes

acjh reviewed Feb 2, 2020

View reviewed changes

acjh approved these changes Feb 3, 2020

View reviewed changes

yash-chowdhary added 8 commits February 3, 2020 11:31

Add icon size classes

dcb5258

Fix size class usage

9c5914e

Remove css classes

3a65565

Remove font-size attribute

92bd7ae

Allow icon-size usage

f2b2e3f

Clean up code

a07ff75

Use computed method to add icon size class

2875ce9

Cleanup code

eb77dd5

yash-chowdhary force-pushed the box-icon-size branch from 28cd48f to eb77dd5 Compare February 3, 2020 03:36

yamgent approved these changes Feb 4, 2020

View reviewed changes

yamgent merged commit 94ca196 into MarkBind:master Feb 4, 2020

yamgent modified the milestones: v2.0.1-markbind.33, v2.0.1-markbind.34 Feb 4, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

TipBox.vue: Support different size icons #124

TipBox.vue: Support different size icons #124

yash-chowdhary commented Jan 22, 2020 •

edited

yash-chowdhary commented Jan 22, 2020

acjh left a comment

acjh Jan 24, 2020

yash-chowdhary Jan 24, 2020

acjh Jan 25, 2020

yash-chowdhary Jan 30, 2020

acjh Jan 30, 2020

yash-chowdhary Jan 30, 2020

yash-chowdhary commented Jan 25, 2020

damithc commented Jan 25, 2020

yash-chowdhary commented Jan 25, 2020

damithc commented Jan 25, 2020

acjh commented Jan 25, 2020

damithc commented Jan 25, 2020

acjh commented Jan 25, 2020

damithc commented Jan 25, 2020

yash-chowdhary commented Jan 30, 2020

acjh left a comment

acjh Jan 30, 2020

acjh Feb 1, 2020

yash-chowdhary Feb 1, 2020

acjh Feb 1, 2020

acjh Feb 2, 2020

acjh Feb 2, 2020

yamgent left a comment •

edited

	<div v-if="!isDefault" class="icon-wrapper" :class="[iconStyle]" >
	<div v-if="!isDefault" class="icon-wrapper" :class="[iconStyle]">

TipBox.vue: Support different size icons #124

TipBox.vue: Support different size icons #124

Conversation

yash-chowdhary commented Jan 22, 2020 • edited

yash-chowdhary commented Jan 22, 2020

acjh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yash-chowdhary commented Jan 25, 2020

damithc commented Jan 25, 2020

yash-chowdhary commented Jan 25, 2020

damithc commented Jan 25, 2020

acjh commented Jan 25, 2020

damithc commented Jan 25, 2020

acjh commented Jan 25, 2020

damithc commented Jan 25, 2020

yash-chowdhary commented Jan 30, 2020

acjh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yamgent left a comment • edited

Choose a reason for hiding this comment

yash-chowdhary commented Jan 22, 2020 •

edited

yamgent left a comment •

edited