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

Update documentation on icon slot for boxes #1123

Merged
merged 3 commits into from
Mar 21, 2020

Conversation

le0tan
Copy link
Contributor

@le0tan le0tan commented Mar 14, 2020

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

• [X] Documentation update

Please replace the vue-strap file with MarkBind/vue-strap#136

Fixes #1030

**Box Slot Options:**
Slot name | Default class | Notes
--- | --- | ---
_icon | (depends on box's `type` attribute) | Example: `<i slot="_icon" class="fas fa-times"></i>` will replace the icon with a cross sign in spite of the box's type.
Copy link
Contributor

Choose a reason for hiding this comment

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

<i slot="_icon" class="fas fa-times"></i> is not how I normally use the slot technique.
I was expecting to use it like <span slot="icon" class="text-danger">:fas-home:</span>. Would that work?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You can use <span slot="_icon" class="text-danger"><md>:fas-home:</md></span>:
image
The only differences are: using _icon instead of icon, using <md> to enforce the markdown parsing inside embedded HTML.

Copy link
Contributor

Choose a reason for hiding this comment

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

Nice.
I suppose not easy to change to icon instead of _icon? If not, this is going to be the first time we expose a underscore prefixed name in the user guide.

Copy link
Contributor

Choose a reason for hiding this comment

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

Nice.
I suppose not easy to change to icon instead of _icon? If not, this is going to be the first time we expose a underscore prefixed name in the user guide.

Should be pretty simple. Having an underscore does not change vue's slot behaviour in anyway.
_xxx is just the naming convention for slots we don't want to be exposed to the user MarkBind/vue-strap#120 (comment)

Copy link
Contributor

Choose a reason for hiding this comment

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

Let's use icon then, as we are now exposing it to the users?

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, are there any limitations as to what can be put in the slot? e.g., can I put a markbind thumnail or an image in the icon slot?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let's use icon then, as we are now exposing it to the users?

Sure, will create corresponding vue-strap PR shortly.

Also, are there any limitations as to what can be put in the slot? e.g., can I put a markbind thumnail or an image in the icon slot?

There shouldn't be any limitations as it's just replacing the placeholder in the template. Just the formatting could be messed up and need manual config.

Copy link
Contributor

Choose a reason for hiding this comment

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

Good. Can try some reasonable use cases (e.g., using thumbnails, images, small animated gifs) as icons and give them as examples in the documentation.

<img slot="icon" src="https://icons8.com/vue-static/landings/animated-icons/icons/cloud/cloud.gif"></img>
some very useful info
</box>

Copy link
Contributor

Choose a reason for hiding this comment

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

Can post a screenshot? I can't see the final look from the PR preview.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

63a0b9f4-8658-4f6d-9077-d8102c6152b2

Copy link
Contributor

Choose a reason for hiding this comment

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

For the animated gif, can find one without background (or make the box background white) so that it blends in nicely? We should try to make the examples as good looking as possible.

Can also add an example using a thumbnail as an icon (i.e., using MarkBind thumbnails feature), possibly a thumbnail of a letter or a number?

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.

Changes to /vue-strap looks good 👍
will need to update the name for the icon attribute to slot transformation, and tests as well here


Slot name | Default class |
--- | --- |
icon | (depends on box's `type` attribute) |
Copy link
Contributor

Choose a reason for hiding this comment

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

Could remove the brackets ( )

--- | --- |
icon | (depends on box's `type` attribute) |

*Example 1*: Override the default icon for a certain type of box.
Copy link
Contributor

Choose a reason for hiding this comment

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

Could use {{ icon_example }} to be consistent with the rest of the page here

@le0tan le0tan requested a review from marvinchin March 21, 2020 09:43
Copy link
Contributor

@marvinchin marvinchin left a comment

Choose a reason for hiding this comment

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

LGTM 🚀 As a heads up, this would break sites that were using the (undocumented) _icon slot.

@marvinchin marvinchin added this to the v2.12.1 milestone Mar 21, 2020
@marvinchin marvinchin merged commit 2ee765b into MarkBind:master Mar 21, 2020
Tejas2805 added a commit to Tejas2805/markbind that referenced this pull request Mar 23, 2020
* 'master' of https://github.com/MarkBind/markbind:
  Update test files (MarkBind#1138)
  Remove OK button from modals (MarkBind#1134)
  Add start from line number functionality to code blocks (MarkBind#1115)
  Allow special tags to be self-closing (MarkBind#1101)
  Simplify baseUrl resolving process (MarkBind#1087)
  Remove redundant file writing (MarkBind#1090)
  Bump acorn from 5.7.3 to 5.7.4 (MarkBind#1121)
  Bump acorn from 7.1.0 to 7.1.1 in /src/lib/markbind (MarkBind#1120)
  Unify markdown-it parser variants (MarkBind#1056)
  Remove dynamic include feature (MarkBind#1037)
  Fix flex shrink not applying in content wrapper (MarkBind#1135)
  Escape Nunjucks for special tags (MarkBind#1049)
  Update documentation on icon slot for boxes (MarkBind#1123)
Tejas2805 added a commit to Tejas2805/markbind that referenced this pull request Mar 23, 2020
…x-pageNav

* 'master' of https://github.com/MarkBind/markbind:
  Update test files (MarkBind#1138)
  Remove OK button from modals (MarkBind#1134)
  Add start from line number functionality to code blocks (MarkBind#1115)
  Allow special tags to be self-closing (MarkBind#1101)
  Simplify baseUrl resolving process (MarkBind#1087)
  Remove redundant file writing (MarkBind#1090)
  Bump acorn from 5.7.3 to 5.7.4 (MarkBind#1121)
  Bump acorn from 7.1.0 to 7.1.1 in /src/lib/markbind (MarkBind#1120)
  Unify markdown-it parser variants (MarkBind#1056)
  Remove dynamic include feature (MarkBind#1037)
  Fix flex shrink not applying in content wrapper (MarkBind#1135)
  Escape Nunjucks for special tags (MarkBind#1049)
  Update documentation on icon slot for boxes (MarkBind#1123)
  Convert code in boxes to code block (MarkBind#1086)
@damithc
Copy link
Contributor

damithc commented Mar 30, 2020

@le0tan perhaps this enhancement got affected by MarkBind/vue-strap#131 ?
image

@le0tan
Copy link
Contributor Author

le0tan commented Mar 30, 2020

@damithc looks like so. As the width of the icon wrapper is fixed, it makes the slot for icon less useful - 1em width is usually not enough for pictures. @yash-chowdhary can you take a look?

@yash-chowdhary
Copy link
Contributor

yash-chowdhary commented Apr 2, 2020

@le0tan perhaps this enhancement got affected by [MarkBind/vue-strap#131]

This issue can be fixed by making use of our icon-size attribute.
Here's the original (w/o icon-size) -
Screenshot 2020-04-02 at 9 54 43 AM

Here's the box with icon-size= "2x" -
Screenshot 2020-04-02 at 9 55 19 AM

Here's an example with multiple boxes -

Screenshot 2020-04-02 at 10 05 10 AM

For @damithc 's example on using thumbnails -
Screenshot 2020-04-04 at 1 56 02 PM

achievable this way -

<box icon-size="2x" type="info">
    <thumbnail circle slot="icon" text=":book:" background="#dff5ff" size="50"/>
    thumbnail as icon
</box>

We can still maintain the width of the icon-wrappers with this enhancement.

@damithc
Copy link
Contributor

damithc commented Apr 6, 2020

For @damithc 's example on using thumbnails -
Screenshot 2020-04-04 at 1 56 02 PM

The thumbnail should be circle.

BTW, if we have to make a choice, I'd rather sacrifice fixed alignment to gain better aspect ratio. Hopefully, we can have both.

@yash-chowdhary
Copy link
Contributor

The solution mentioned in #1172 would fix this.
Vuestrap PR link

marvinchin pushed a commit that referenced this pull request Apr 10, 2020
* Update documentation on icon slot for boxes

* Update the examples for using icon slot for boxes.

* Resolve comments.
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.

<box>: Allow slot support for icons
5 participants