Skip to content
This repository has been archived by the owner on Jul 12, 2020. It is now read-only.

Insert addClass prop to all vue-strap components #84

Merged
merged 11 commits into from
Jul 25, 2018

Conversation

Chng-Zhi-Xuan
Copy link

@Chng-Zhi-Xuan Chng-Zhi-Xuan commented Jul 24, 2018

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

• [x] New feature

Part of MarkBind/markbind#363

What is the rationale for this request?
Add requested feature

What changes did you make? (Give an overview)

  • Add customClass prop to all components.
  • Tabs component is split into 3 distinct sub-components, customClass added to all 3.
  • EDIT Tabs custom class is only added to the root / parent component as we discussed there is no need for the child sub components to have it.

Testing instructions:

  1. Do npm run build and copy vue-strap.min.js to your markbind asset folder
  2. Insert the following test code for all components into index.md, custom classes should be added to the outer most wrapper of each component.
<panel header="doot doot" custom-class="meow woof doot">
  XYZ
</panel>

<dropdown text="Action" type="primary" custom-class="meow woof doot">
  <li><a href="#dropdown" class="dropdown-item">Action</a></li>
  <li><a href="#dropdown" class="dropdown-item">Another action</a></li>
  <li><a href="#dropdown" class="dropdown-item">Something else here</a></li>
  <li role="separator" class="dropdown-divider"></li>
  <li><a href="#dropdown" class="dropdown-item">Separated link</a></li>
</dropdown>

<navbar placement="top" type="inverse" custom-class="meow woof doot">
  <!-- Brand as slot -->
  <a slot="brand" href="/" title="Home" class="navbar-brand">MarkBind</a>
  <!-- You can use dropdown component -->
  <dropdown text="Dropdown" class="nav-link">
    <li><a href="#navbar" class="dropdown-item">Option</a></li>
  </dropdown>
  <!-- For right positioning use slot -->
  <li slot="right">
    <a href="https://github.com/MarkBind/markbind" target="_blank" class="nav-link">Fork...</a>
  </li>
</navbar>

<pic src="https://vuejs.org/images/logo.png" width="300" alt="Logo" custom-class="meow woof doot">
  Logo for Vue.js
</pic>

<popover effect="fade" content="Lorem ipsum dolor sit amet" placement="top" custom-class="meow woof doot">
  <button class="btn btn-secondary">Popover on top</button>
</popover>

<question custom-class="meow woof doot">
  The question body. <md>:grey_question:</md>
  <div slot="hint">
    Question hint. <md>:crystal_ball:</md>
  </div>
  <div slot="answer">
    Question answer. <md>:pencil:</md>
  </div>
</question>

<tabs custom-class="meow woof doot">
  <tab header="First tab" custom-class="meow woof doot">
    ...
  </tab>
  <tab header="Disabled second tab :x:" disabled custom-class="meow woof doot"> 
  </tab>
  <tab-group header="Third tab group :milky_way:" custom-class="meow woof doot">
    <tab header="Stars :star:" custom-class="meow woof doot">
      star facts 
    </tab>
    <tab header="Disabled Moon :new_moon:" disabled custom-class="meow woof doot">
    </tab>
  </tab-group custom-class="meow woof doot">
  <tab-group header="Disabled fourth tab group" disabled>
    <tab header="Hidden tab" custom-class="meow woof doot">
      ...
    </tab>
  </tab-group>
</tabs>

<tip-box type="info" custom-class="meow woof doot">
    info
</tip-box>

<tooltip header content="Lorem ipsum dolor sit amet" placement="top" custom-class="meow woof doot">
  <button class="btn btn-secondary">Popover on top</button>
</tooltip>

<span>
  More about <trigger for="tt:trigger_id" custom-class="meow woof doot" trigger="click">trigger</trigger>.
</span>
<tooltip id="tt:trigger_id" content="This tooltip triggered by a trigger"></tooltip>

More about <trigger for="modal:trigger_id2" trigger="click">trigger</trigger>.
<modal title="**Modal title** :rocket:" id="modal:trigger_id2" custom-class="meow woof doot">
    ...
</modal>

@damithc
Copy link

damithc commented Jul 24, 2018

Use a shorter name (or give a shorter alias) e.g., add="..." ? We want to keep the syntax concise.

src/Tab.vue Outdated
@@ -21,6 +21,10 @@ export default {
disabled: {
type: Boolean,
default: false
},
customClass: {
Copy link
Member

Choose a reason for hiding this comment

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

 <tab header="First tab" custom-class="meow woof doot">
    ...
  </tab>
  <tab header="Disabled second tab :x:" disabled custom-class="meow woof doot"> 
  </tab>

Tabs and TabGroups doesn't seem to have the correct classes, they did not appear in the html when I checked it.

Copy link
Author

@Chng-Zhi-Xuan Chng-Zhi-Xuan Jul 24, 2018

Choose a reason for hiding this comment

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

After refactoring the classes to be inserted within Tabset.vue, it will appear. However, do we really need to put the blanket class on all child sub-components?

If so, we would need to modify Panel and Question among others as well.

I feel just applying it to the outer most wrapper is sufficient.

Copy link
Member

Choose a reason for hiding this comment

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

However, do we really need to put the blanket class on all child sub-components?

Not needed actually, don't really see a use case for it.

@Chng-Zhi-Xuan
Copy link
Author

Update

  • Shortened customClass to cc
  • Undo changes for Tab and TabGroup

@yamgent
Copy link
Member

yamgent commented Jul 24, 2018

cc is now too short, not very obvious what it stands for. Maybe add-cls or add-class?

@damithc
Copy link

damithc commented Jul 24, 2018

cc is now too short, not very obvious what it stands for. Maybe add-cls or add-class?

I guess we can't use class? In that case I'm OK with add-class.

@Chng-Zhi-Xuan
Copy link
Author

Chng-Zhi-Xuan commented Jul 24, 2018

Update

  • Change custom class name to addClass / add-class
  • Shift custom class name position in Modal
  • Squashed changes

@Chng-Zhi-Xuan Chng-Zhi-Xuan changed the title Add customClass prop to all vue-strap components Insert addClass prop to all vue-strap components Jul 24, 2018
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.

One minor nit:

src/Navbar.vue Outdated
@@ -1,5 +1,6 @@
<template>
<nav ref="navbar" :class="['navbar', 'navbar-expand-md', {
<nav ref="navbar" :class="['navbar', 'navbar-expand-md', addClass,
Copy link
Member

Choose a reason for hiding this comment

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

The addClass should be at the back of the array.

@Chng-Zhi-Xuan
Copy link
Author

Update

  • Squashed and force pushed

@yamgent yamgent added this to the v2.0.1-markbind.16 milestone Jul 24, 2018
@acjh
Copy link

acjh commented Jul 24, 2018

Does class not work?

@Chng-Zhi-Xuan
Copy link
Author

Chng-Zhi-Xuan commented Jul 24, 2018

I did not try, but I think it could work.

However there are 2 small issues with it:

  • Maintainability in the .vue file. Having a prop named "class" along with the v-bind syntax with HTML's class might be confusing for future devs.
  • It also might be confusing to the author since on our end we are adding classes behind the scenes. What custom class the author specifies will not be the only class being tied to that component.

Renaming it to add-class implies that it is an "addition" to existing classes instead of "replacing".

@yamgent yamgent merged commit 0c8e3f6 into MarkBind:master Jul 25, 2018
@Chng-Zhi-Xuan Chng-Zhi-Xuan deleted the 363-custom-classes branch May 27, 2019 02:33
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants