Enable seamless Panel's caret to stay inline #74

Chng-Zhi-Xuan · 2018-07-17T06:26:46Z

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

• [x] Enhancement to an existing feature

What is the rationale for this request?
Seamless Panels have its caret pushed to the previous line when the Panel width is too small. The expected behaviour is having the header text wrap around while having the caret inline.

What changes did you make? (Give an overview)

Added a new toggle-able inline caret to default header slot rendered via v-html="headerContent".
Cleaned up some logic for existing caret.
Change popup button to only appear on mouse over for seamless Panels.

Before	After

Note
When using `slot="header"` , the problem will re-emerge as there is no way to insert the caret inside the slotted element without using `Cheerio / JQuery`.

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

Logic in methods and template

Testing instructions:

Run npm run build and paste the updated vue-strap.min.js into MarkBind's asset folder.
Run markbind init, author a seamless panel with a long header.
Run markbind serve and reduce window width.
Check that the caret stays inline and is toggle-able.

yamgent · 2018-07-17T07:12:50Z

The popup button shows even if no popup url is specified.

<panel type="seamless" header="The The The The The The The The The The ">
Hi.
</panel>

yamgent · 2018-07-17T08:05:02Z

src/Panel.vue

                                @click.stop="close()">
                            <span class="glyphicon glyphicon-remove" aria-hidden="true"></span>
                        </button>
                        <button type="button" :class="['popup-button', 'btn', isLightBg ? 'btn-outline-secondary' : 'btn-outline-light']"
-                                v-show="this.popupUrl !== null"
+                                v-show="((this.popupUrl !== null) && (isSeamless ? onHeaderHover : (!noCloseBool)))"


As discussed, if no-close is specified, but popup is specified, the popup button still does not show. The user might have reasons to show the popup button even if he wants to hide the close button.

yamgent · 2018-07-17T08:08:44Z

src/Panel.vue

+        if (this.isSeamless) {
+            return this.caretHtml + ' ' + this.inlineRenderedHeader;
+        }
+        return this.renderedHeader;


As discussed, maybe everything can just use inline then, since for non-seamless, it seems to not make a difference, there is no reason to do different rendering ways for different type of panels.

yamgent

Some minor nits

yamgent · 2018-07-17T08:25:59Z

src/Panel.vue

                                @click.stop="close()">
                            <span class="glyphicon glyphicon-remove" aria-hidden="true"></span>
                        </button>
                        <button type="button" :class="['popup-button', 'btn', isLightBg ? 'btn-outline-secondary' : 'btn-outline-light']"
-                                v-show="this.popupUrl !== null"
+                                v-show="((this.popupUrl !== null) && (isSeamless ? onHeaderHover : true))"


(isSeamless ? onHeaderHover : true)) is a form of p -> q.

Therefore, this can be simplified to:

(this.popupUrl !== null) && (!isSeamless || onHeaderHover)

yamgent · 2018-07-17T08:29:27Z

src/Panel.vue

+        }
+      },
+      hasCustomHeader () {
+        if (this.$slots.header) {


Can this be simplified to return this.$slots.header;?

Then it will return either a null object or a v-node object, which is not a Boolean

How about return this.$slots.header !== null; then

I was wrong, it gives undefined instead of null. Will modify accordingly.

Chng-Zhi-Xuan · 2018-07-18T06:49:18Z

Update

Squashed commits

damithc · 2018-07-20T15:43:16Z

This problem seems to be fixed in most places but traces of it remain in some places. e.g.,

The above is a new bit of code I added today, BTW.

yamgent · 2018-07-23T01:16:25Z

This problem seems to be fixed in most places but traces of it remain in some places. e.g.,

That panel header appears that way because it uses a custom header slot.

We did not implement the logic to insert the caret inside the content, because the content here can literally be anything, so we cannot make any good assumptions. Hence, custom headers will always have the caret outside the content, ~~resulting in the breaking.~~ (new discovery, see subsequent post).

See the original post:

When using slot="header" , the problem will re-emerge as there is no way to insert the caret inside the slotted element without using Cheerio / JQuery.

acjh · 2018-07-23T01:20:48Z

Try styling the slot as display: inline-block;

yamgent · 2018-07-23T01:35:35Z

Try styling the slot as display: inline-block;

Tried it, didn't work. :S

damithc · 2018-07-23T03:20:00Z

I see. Looks like there is no easy way around it. Still, we should not give up entirely. I use slots a lot because my headings often contains span elements (e.g., to add stars of a certain color). Would the introduction of a <header> element help?

May be create a separate issue and explain the exact problem?

damithc · 2018-07-23T05:17:29Z

BTW, can we allocate more space for the header (and less space for panel buttons)? That should reduce the incidence of this problem. In this screenshot, there seems to be enough space for the full title.

yamgent · 2018-07-23T06:18:21Z

Found the reason why the caret breaks on your website. It is because your custom headers were using the card-title Bootstrap style, but it should not be used when the panels are seamless, because we don't use the Bootstrap's card design when it is seamless.

Solution: If custom header slots are used, and your panels are seamless, don't use card-title style.

~~This solution will have to be documented in the "Advanced Tips and Tricks" section.~~

As to why I was able to somehow "replicate" the issue on my side, I realize that this does not happen if the custom slot header is declared with <span>. I was using <p> all along, which explains why the caret and the headers break on my side. My bad. :P

yamgent · 2018-07-23T06:29:57Z

@damithc we found another issue with the implementation of caret with the panel that may be related to this bug, so you can ignore the above solution for now, as we will try to fix it on our side.

…s-panel" This reverts commit d7626b3, reversing changes made to a0bae9e.

@nusjzx

The caret is combined with the header content. It is usually rendered like this: > Some heading However, custom header contents, after markdown rendering, will be wrapped with block HTML tags such as <p> and <h1>, which will make browsers render a line break between the caret and header content: > Some heading Let's fix this by moving the caret to a separate div. Then, make everything inside a panel header be on a single line by using CSS 'display: inline-block' and 'width: ...', such that the width adds up to 100%. +--------+-----------------+---------+ | caret | header-content | buttons | | (32px) | (100% - 32 - 32)| (32px) | +--------+-----------------+---------+ Thanks to @nusjzx for providing the fix to the button-wrapper in PR MarkBind#86, in which his use of 'calc(100% - 32px)' provided a source of inspiration for this fix. His fix in that PR is incorporated into this commit because without it, the responsive design will not work. An alternative fix considered was to use md.renderInline(). However, md.renderInline() does not render markdown headings. Another alternative fix was to insert the caret directly inside the header content. However, after PR MarkBind#74, MarkBind#81 and MarkBind#83 were merged, new problems continue to emerge. These problems are reported as comments in these pages: - MarkBind#74 - MarkBind#81 - MarkBind#83 - MarkBind/markbind#373 - MarkBind/markbind#383 As such, the PRs for this alternative fix has been reverted by the previous commits before this commit. Therefore, this commit shall be viewed as the canonical fix for issue MarkBind/markbind#337.

@nusjzx

The caret is combined with the header content. It is usually rendered like this: > Some heading However, custom header contents, after markdown rendering, will be wrapped with block HTML tags such as <p> and <h1>, which will make browsers render a line break between the caret and header content: > Some heading Let's fix this by moving the caret to a separate div. Then, make everything inside a panel header be on a single line by using CSS 'display: inline-block' and 'width: ...', such that the width adds up to 100%. +--------+-----------------+---------+ | caret | header-content | buttons | | (32px) | (100% - 32 - 32)| (32px) | +--------+-----------------+---------+ Thanks to @nusjzx for providing the fix to the button-wrapper in PR MarkBind#86, in which his use of 'calc(100% - 32px)' provided a source of inspiration for this fix. His fix in that PR is incorporated into this commit because without it, the responsive design will not work. An alternative fix considered was to use md.renderInline(). However, md.renderInline() does not render markdown headings. Another alternative fix was to insert the caret directly inside the header content. However, after PR MarkBind#74, MarkBind#81 and MarkBind#83 were merged, new problems continue to emerge. These problems are reported as comments in these pages: - MarkBind#74 - MarkBind#81 - MarkBind#83 - MarkBind/markbind#373 - MarkBind/markbind#383 As such, the PRs for this alternative fix has been reverted by the previous commits before this commit.

@nusjzx

The caret is combined with the header content. It is usually rendered like this: > Some heading However, custom header contents, after markdown rendering, will be wrapped with block HTML tags such as <p> and <h1>, which will make browsers render a line break between the caret and header content: > Some heading Let's fix this by moving the caret to a separate div. Then, make everything inside a panel header be on a single line by using CSS 'display: inline-block' and 'width: ...', such that the width adds up to 100%. +--------+-----------------+---------+ | caret | header-content | buttons | | (32px) | (100% - 32 - 32)| (32px) | +--------+-----------------+---------+ Thanks to @nusjzx for providing the fix to the button-wrapper in PR MarkBind#86, in which his use of 'calc(100% - 32px)' provided a source of inspiration for this fix. His fix in that PR is incorporated into this commit because without it, the responsive design will not work. An alternative fix considered was to use md.renderInline(). However, md.renderInline() does not render markdown headings. Another alternative fix was to insert the caret directly inside the header content. However, after PR MarkBind#74, MarkBind#81 and MarkBind#83 were merged, new problems continue to emerge. These problems are reported as comments in these pages: - MarkBind#74 - MarkBind#81 - MarkBind#83 - MarkBind/markbind#373 - MarkBind/markbind#383 As such, the PRs for this alternative fix has been reverted by the previous commits before this commit.

Chng-Zhi-Xuan changed the title ~~Panel: Enable seamless caret to stay inline~~ Enable seamless Panel's caret to stay inline Jul 17, 2018

Chng-Zhi-Xuan mentioned this pull request Jul 17, 2018

Improve MarkBind's responsive / mobile usability MarkBind/markbind#335

Closed

8 tasks

yamgent reviewed Jul 17, 2018

View reviewed changes

yamgent approved these changes Jul 18, 2018

View reviewed changes

yamgent added this to the v2.0.1-markbind.15 milestone Jul 18, 2018

Panel: Enable seamless caret to stay inline

096ecaf

Chng-Zhi-Xuan force-pushed the 337-seamless-panel branch from 60d4d6f to 096ecaf Compare July 18, 2018 06:48

yamgent merged commit d7626b3 into MarkBind:master Jul 19, 2018

yamgent mentioned this pull request Jul 20, 2018

Panel: Change header markdown rendering to non-inline #81

Merged

yamgent mentioned this pull request Jul 23, 2018

Enable seamless Panel's caret to stay inline for custom header slots #83

Merged

yamgent added a commit to yamgent/vue-strap that referenced this pull request Jul 27, 2018

Revert "Merge pull request MarkBind#74 from Chng-Zhi-Xuan/337-seamles…

b11037f

…s-panel" This reverts commit d7626b3, reversing changes made to a0bae9e.

yamgent mentioned this pull request Jul 27, 2018

Fix seamless panel caret #88

Merged

Chng-Zhi-Xuan deleted the 337-seamless-panel branch May 27, 2019 02:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable seamless Panel's caret to stay inline #74

Enable seamless Panel's caret to stay inline #74

Chng-Zhi-Xuan commented Jul 17, 2018 •

edited

Loading

yamgent commented Jul 17, 2018

yamgent Jul 17, 2018

yamgent Jul 17, 2018

yamgent left a comment

yamgent Jul 17, 2018

acjh Jul 17, 2018

yamgent Jul 17, 2018

Chng-Zhi-Xuan Jul 17, 2018

yamgent Jul 17, 2018

Chng-Zhi-Xuan Jul 18, 2018

Chng-Zhi-Xuan commented Jul 18, 2018

damithc commented Jul 20, 2018

yamgent commented Jul 23, 2018 •

edited

Loading

acjh commented Jul 23, 2018

yamgent commented Jul 23, 2018

damithc commented Jul 23, 2018

damithc commented Jul 23, 2018

yamgent commented Jul 23, 2018 •

edited

Loading

yamgent commented Jul 23, 2018

Enable seamless Panel's caret to stay inline #74

Enable seamless Panel's caret to stay inline #74

Conversation

Chng-Zhi-Xuan commented Jul 17, 2018 • edited Loading

yamgent commented Jul 17, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yamgent 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

Chng-Zhi-Xuan commented Jul 18, 2018

damithc commented Jul 20, 2018

yamgent commented Jul 23, 2018 • edited Loading

acjh commented Jul 23, 2018

yamgent commented Jul 23, 2018

damithc commented Jul 23, 2018

damithc commented Jul 23, 2018

yamgent commented Jul 23, 2018 • edited Loading

yamgent commented Jul 23, 2018

Chng-Zhi-Xuan commented Jul 17, 2018 •

edited

Loading

yamgent commented Jul 23, 2018 •

edited

Loading

yamgent commented Jul 23, 2018 •

edited

Loading