Enhance box component heading by ang-zeyu · Pull Request #118 · MarkBind/vue-strap

ang-zeyu · 2019-12-29T09:07:01Z

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#962

Requires MarkBind/markbind#1025

What is the rationale for this request?

Update box header styles as discussed in x of the dismissible boxes it not at the top-right corner markbind#962, and Add bootswatch theme specific tip box styles markbind#999, and integration with seamless style boxes ( see gif, undiscussed in links )
Fix a minor issue with seamless style boxes when used with the light option

What changes did you make? (Give an overview)

Box heading style changes

Change box headings to use slots for markdown parsing added in Implement box markdown header attributes parsing markbind#1025
Fix display issue with seamless and light boxes being used together

Provide some example code that this change will affect:

Template of the header wrapper

...
<div v-if="headerBool" :class="['box-header-wrapper', { 'alert-dismissible': dismissible }]">
    <div v-show="!isDefault" class="icon-wrapper" :class="[iconStyle]">
        <slot name="_icon">
            <span v-html="iconType"></span>
        </slot>
    </div>
    <div class="box-header">
       <slot name="_header"></slot>
    </div>
    <button v-show="dismissible" type="button" class="close close-with-heading" data-dismiss="alert" aria-label="Close">
        <span aria-hidden="true">&times;</span>
   </button>
</div>
...

Testing instructions:

use this template file to test the combinations in the gif, with some extra

Proposed commit message: (wrap lines at 72 characters)
Enhance box component heading

Box headings are anchored to the top right corner of the box.
Such a heading style causes the box content to overflow at times.

Using the new seamless option with the light option causes unintended
output as well.

Let’s enhance the box component heading style, using a separate top
horizontal section instead, fixing the content overflow.

In addition, let’s support markdown parsing for the header, which pairs
Nicely with the new support for various icon sizes.

Lastly, let’s give the light option priority over the seamless option if
both options are mistakenly used.

yamgent · 2020-01-01T02:24:20Z

src/TipBox.vue

+        border-radius: 6px 6px 0 0;
+    }
+
+    .content-wrapper {


While this is a class, in MarkBind, we also use this name as an ID to wrap the entire webpage. To avoid confusion, use tipbox-body instead?

yamgent · 2020-01-01T02:37:11Z

src/TipBox.vue

        default: ''
      },
-      heading: {
+      header: {


Potential breaking change (some people might have already started using the new heading feature, which means their site will break immediately). My suggestion is we support both first while deprecating heading.

yamgent · 2020-01-01T02:38:11Z

src/TipBox.vue

+    div.box-heading > * {
+        margin-bottom: 0;
+    }
+</style>


Also just curious, is there a reason why we are using a separate <style> section?

ang-zeyu · 2020-01-02T06:10:07Z

Thanks for looking through it! Will update the relavant sections

The separate <style> section is a temporary solution for the lack of deep selectors for scoped css in the current vue-loader version being used, until it is upgraded.
I've looked into upgrading it briefly, and the deep selectors at least seems to work fine in the new version, but I'm not as familiar with tweaking webpack plugins / settings, and would rather do it in a follow-up PR which would be easier to rollback.

Is this a good solution to theme specific styles as well?

Hence, perhaps https://github.com/thomaspark/bootswatch ( under MIT license ) could be forked / patched, and the stylesheets extended. Though, some of the code handling themes have to be changed.

An alternative is to hardcode another layer of stylesheets based on which is currently in use, which can make it clearer which styles were changed / added from bootswatch styles, but it may add some unnecessary complexity to the code

yamgent · 2020-01-03T01:07:21Z

I've looked into upgrading it briefly, and the deep selectors at least seems to work fine in the new version, but I'm not as familiar with tweaking webpack plugins / settings, and would rather do it in a follow-up PR which would be easier to rollback.

Ok let's do it in a subsequent PR then. We strife for simplicity for now.

ang-zeyu · 2020-02-04T18:33:55Z

Updated styles as discussed MarkBind/markbind#999

Added a thin horizontal divider when used with seamless as well

marvinchin

The code itself looks fine to me 🙂 I tried to use your template to build a site to test the feature, but the table looks broken on my end:

This is with the vue-strap.min.js build from this PR, and MarkBind checked out from the branch in MarkBind/markbind#1025.

Do you mind testing out the template again to see if the error is reproducible on your environment?

marvinchin · 2020-02-13T15:29:40Z

src/TipBox.vue

      },
      isSeamless() {
-        return toBoolean(this.seamless);
+        return !this.light && this.seamless;


Just wondering if we should fail hard or emit an error message if both light and seamless is present. It feels like it might make debugging easier for the user when trying to figure out why the output might not be what he is expecting.

Just wondering if we should fail hard or emit an error message if both light and seamless is present. It feels like it might make debugging easier for the user when trying to figure out why the output might not be what he is expecting.

I think emitting an error message is a good idea, we could that in the new componentParser in MarkBind easily

We could do this in a seperate PR with warnings for deprecated attributes in one go as well, something like "Add warning messasges for inconsistent component attributes" perhaps

I don't mind updating MarkBind/markbind#1025 for the light vs seamless thing alone first though, let me know which would be better 😄

If it's not a lot of effort, it would be great to add that in before we merge that in 🙂

I think it would be better that we write a common utility to check for this. Since both #129 and this have the possibility of inconsistent attributes. I can either include it in my PR, although I'm more inclined that it'd be in a seperate PR altogether.

Adding a common utility in a common PR after both these PRs are merged and applying it to all the new changes at once sounds good to me. Let's open an issue to track this.

Perhaps we can leave some // TODOs to indicate where these checks should happen to make it easier for the person working on this issue.

Perhaps we can leave some // TODOs to indicate where these checks should happen to make it easier for the person working on this issue.

Updated MarkBind/markbind#1025 with it

ang-zeyu · 2020-02-14T09:51:57Z

Do you mind testing out the template again to see if the error is reproducible on your environment?

Thanks for looking through this!

I just tried it again on MarkBind/markbind#1025 as well, it appears as is in the gif.
The template file I linked is the entire template file , not meant to be inserted in there, could that be the cause? ( double frontmatter may cause issues )

marvinchin

So it turned out that VSCode was doing some autoformatting that messed up my files 🙃It looks pretty good to me, just a few more questions.

marvinchin · 2020-02-18T07:47:56Z

src/TipBox.vue

      },
      isSeamless() {
-        return toBoolean(this.seamless);
+        return !this.light && this.seamless;


If it's not a lot of effort, it would be great to add that in before we merge that in 🙂

marvinchin · 2020-02-18T08:00:19Z

src/TipBox.vue

-      headerContent() {
-        return this.header || this.heading;
+      headerBool() {
+        return this.$slots._header;


this.$slots._header seems to be an object. headerBool feels like a misnomer here. I think we should either keep it as headerContent, or make it return a boolean.

Also, we seem to have two different naming conventions for this component: isXXX vs xxxBool. Should we stick to one of them?

Also, we seem to have two different naming conventions for this component: isXXX vs xxxBool. Should we stick to one of them?

Hmm, I followed the convention in panelBase.js for this, as its a little strange to call it isHeader. I could change it to hasHeader if preferred though!

If it's not a lot of effort, it would be great to add that in before we merge that in 🙂

I can't comment in the above comment for this strangely 😕

I removed toBoolean as our docs dosen't show using light/seamless="false", and
this.light is also used directly for lightStyle ( light="false" = truthy )

this.$slots._header seems to be an object. headerBool feels like a misnomer here. I think we should either keep it as headerContent, or make it return a boolean.

fixed 👍

marvinchin · 2020-02-18T08:00:34Z

src/TipBox.vue

            case 'success':
            case 'tip':
-                return 'border-sucess text-success alert-border-left';
+              return 'border-success text-success alert-border-left';


Good catch 👀

marvinchin

LGTM :)

Box headings are anchored to the top right corner of the box. Such a heading style causes the box content to overflow at times. Using the new seamless option with the light option causes unintended output as well. Let’s enhance the box component heading style, using a separate top horizontal section instead, fixing the content overflow. In addition, let’s support markdown parsing for the header, which pairs Nicely with the new support for various icon sizes. Lastly, let’s give the light option priority over the seamless option if both options are mistakenly used.

ang-zeyu · 2020-02-19T13:55:59Z

Rebased on latest, no changes

ang-zeyu force-pushed the box-heading-tweaks branch from f78f246 to 1d59578 Compare December 30, 2019 03:14

yamgent reviewed Jan 1, 2020

View reviewed changes

This was referenced Jan 25, 2020

Extra bootswatch theme specific styles MarkBind/markbind#997

Closed

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

Closed

ang-zeyu changed the title ~~[WIP] Tweak box component heading~~ Tweak box component heading Jan 26, 2020

ang-zeyu force-pushed the box-heading-tweaks branch from 1d59578 to b5132ef Compare January 26, 2020 15:40

ang-zeyu mentioned this pull request Feb 4, 2020

doc: seamless style boxes MarkBind/markbind#1019

Merged

ang-zeyu force-pushed the box-heading-tweaks branch 3 times, most recently from b7b1032 to ee33884 Compare February 4, 2020 17:48

ang-zeyu mentioned this pull request Feb 4, 2020

Implement box markdown header attributes parsing MarkBind/markbind#1025

Merged

ang-zeyu changed the title ~~Tweak box component heading~~ Enhance box component heading Feb 4, 2020

ang-zeyu force-pushed the box-heading-tweaks branch from ee33884 to 579de6b Compare February 4, 2020 18:31

ang-zeyu force-pushed the box-heading-tweaks branch from 579de6b to f61a637 Compare February 4, 2020 18:42

marvinchin reviewed Feb 13, 2020

View reviewed changes

marvinchin suggested changes Feb 18, 2020

View reviewed changes

marvinchin mentioned this pull request Feb 18, 2020

Implement no-* attributes for <box> #129

Merged

ang-zeyu force-pushed the box-heading-tweaks branch from f61a637 to dc90a15 Compare February 18, 2020 08:34

marvinchin approved these changes Feb 18, 2020

View reviewed changes

ang-zeyu mentioned this pull request Feb 18, 2020

Implement warning logging when inconsistent or deprecated attributes are used MarkBind/markbind#1048

Closed

ang-zeyu force-pushed the box-heading-tweaks branch from dc90a15 to 154a1e3 Compare February 19, 2020 13:55

ang-zeyu mentioned this pull request Feb 20, 2020

Update dependencies and restructure webpack config #130

Merged

yamgent merged commit dcec9de into MarkBind:master Feb 22, 2020

yamgent added this to the v2.0.1-markbind.36 milestone Feb 22, 2020

nbriannl mentioned this pull request Feb 27, 2020

Add warnings for conflicting/deprecated component attribs MarkBind/markbind#1057

Merged

Conversation

ang-zeyu commented Dec 29, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ang-zeyu commented Jan 2, 2020

Uh oh!

yamgent commented Jan 3, 2020

Uh oh!

ang-zeyu commented Feb 4, 2020

Uh oh!

marvinchin left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ang-zeyu commented Feb 14, 2020

Uh oh!

marvinchin left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

marvinchin left a comment

Choose a reason for hiding this comment

Uh oh!

ang-zeyu commented Feb 19, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

ang-zeyu commented Dec 29, 2019 •

edited

Loading