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

Implement no-* attributes for <box> #129

Merged
merged 4 commits into from
Mar 8, 2020
Merged

Conversation

nbriannl
Copy link

@nbriannl nbriannl commented Feb 12, 2020

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

• [X] Enhancement to an existing feature

Fixes MarkBind/markbind#978
Requires MarkBind/markbind#1042
Requires #118 (will wait before it gets merged)

What is the rationale for this request?

Support no-icon, no-background, no-border attributes helps to omit the respective aspects more conveniently.

What changes did you make? (Give an overview)

When no-icon is absent or icon slot is filled, the icon div will be rendered.
Because it is an or statement, the presence of icon="..." will take precedence over no-icon

When no-background or no-border is applied, .no-background or a .no-border CSS class is applied respectively. Due to the position of CSS rules, it overrides styles applied by bootstrap or by ourselves. Since customs style are applied afterwards via style="...", it is applied last. Thus any style defined by background, border-color, border-left-color take precedence over no-background or no-border.

Corresponding documentation in MarkBind/markbind#1042

Provide some example code that this change will affect:

You can view the syntax at https://gist.github.com/nbriannl/9369aab530e032bc93070cd8958949e3
and the corresponding output at https://nbriannl.github.io/markbind-plain-site/

Is there anything you'd like reviewers to focus on?

Proposed commit message: (wrap lines at 72 characters)

Implement no-* attributes for

@nbriannl nbriannl marked this pull request as ready for review February 14, 2020 07:51
@nbriannl
Copy link
Author

Ready for review

src/TipBox.vue Outdated
Comment on lines 192 to 214
hasBackground() {
if (this.noBackground) {
return 'no-background';
}
},
hasBorder() {
if (this.noBorder) {
return 'no-border';
}
Copy link

Choose a reason for hiding this comment

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

Probably backgroundStyle and borderStyle are more consistent names?

Copy link
Author

@nbriannl nbriannl Feb 16, 2020

Choose a reason for hiding this comment

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

It would be deceiving to say these style the backgrounds and border, since that's what customStyle does. I will rename it noBackgroundStyle and noBorderStyle because they enforce the styles of having no background or no border. Thanks for the heads up.

src/TipBox.vue Outdated
@@ -1,6 +1,6 @@
<template>
<div class="alert container" :class="[boxStyle, addClass, lightStyle, seamlessStyle]" :style="customStyle">
<div v-if="!isDefault" class="icon-wrapper" :class="[iconStyle]">
Copy link
Author

@nbriannl nbriannl Feb 16, 2020

Choose a reason for hiding this comment

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

Previously, the code renders the icon-wrapper div if the type of box is not a default box. However, I change this because the type of the box can still be the default type, and still have an icon (inserted via icon="...").

Example:

<box icon=":rocket:">
    Lorem ipsum dolor sit amet,
</box>

image

@@ -167,15 +181,23 @@
return '<i class="fas fa-lightbulb"></i>';
case 'definition':
return '<i class="fas fa-atlas"></i>';
default:
return '<i class="fas fa-exclamation"></i>';
Copy link
Author

Choose a reason for hiding this comment

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

Default boxes do not have icons.

@nbriannl nbriannl requested a review from yamgent February 16, 2020 07:44
Copy link

@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.

Looks mostly good! Just a couple of suggestions, but nothing major 🙂

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

Choose a reason for hiding this comment

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

I wonder if this will make this part less verbose:

return this.noBackground? 'no-background' : '';

Copy link
Author

@nbriannl nbriannl Feb 26, 2020

Choose a reason for hiding this comment

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

I made it consistent to the coding style of other computed properties. For example,

      seamlessStyle() {
        if (this.isSeamless) {
          return 'seamless';
        }
        return '';
      },
      fontBlack() {
        if (this.light) {
          return 'font-black';
        }
        return '';
      },

@nbriannl nbriannl changed the title Implement no-* attributes for <box> [WIP] Implement no-* attributes for <box> Feb 22, 2020
Implement positive attribute higher priority

Remove redundant icon type

Update code based on comments
@nbriannl nbriannl changed the title [WIP] Implement no-* attributes for <box> Implement no-* attributes for <box> Feb 26, 2020
@nbriannl
Copy link
Author

#118 has been merged. Ready for (re)review.

Copy link

@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.

Seems like there is a typo :P

src/TipBox.vue Outdated
@@ -1,7 +1,7 @@
<template>
<div class="alert box-container" :class="[boxStyle, addClass, lightStyle, seamlessStyle]" :style="customStyle">
<div class="alert box-container" :class="[boxStyle, addClass, lightStyle, seamlessStyle, , noBackgroundStyle, noBorderStyle]" :style="customStyle">

Choose a reason for hiding this comment

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

There's a double , here

Copy link
Author

Choose a reason for hiding this comment

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

😞 Fixed.

@nbriannl nbriannl requested a review from marvinchin February 27, 2020 03:50
Copy link

@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

@marvinchin marvinchin added this to the v2.0.1-markbind.37 milestone Feb 27, 2020
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.

<box>: support no-* attributes
4 participants