Enhance appearance of minimised panels #1694

kaixin-hc · 2021-12-18T14:54:31Z

What is the purpose of this pull request?

Referring to PR #1422 and the associated comments, I wanted to try putting them on the same line as well as using a different icon / sizing / margin. Provides a slightly different suggestion for #1201 .

Overview of changes:
Before

hhdqirui suggested change

Suggested

Add caret icon (as most minimised panels tend to use a caret icon to show that it is a clickable button
Add top margin (This is to prevent ugly cluttering. as expanded cards also have a top margin but not a bottom margin, top margin was chosen. I do wonder what the reason is for having a top margin and not a bottom margin though! )

Anything you'd like to highlight / discuss:

Could be expanded by allowing the user to turn off the icon the same way they can turn off the buttons for the panels
I think this may not pass tests - I found a diff in markbind/fontawesome/css/all.min.css. Is it possible just to run a command to update specific test files or only the ones which have changed...? **ETA: oh, it passed! Huh. I wonder why npm run test stopped then 🤔 **

(Possibly to break up into other PRs?)

I think for code maintenance, we could probably remove some of the styles in NestedPanel.vue . If I remember correctly, we only need to specify the styles to change in the responsive breakpoints, so is it really needed to specify the same style twice?
https://developer.mozilla.org/en-US/docs/Web/CSS/float suggests that it might be unneeded to specify display: inline-block if we are also specifying float, as it would be overridden and ignored.

Testing instructions:

Proposed commit message: (wrap lines at 72 characters)
Enhance appearance of minimised panels

Add caret icon (as most minimised panels tend to use a caret icon to
show that it is a clickable button
Add top margin (as expanded cards also have a top margin but not a
bottom margin, top margin was chosen. This is to prevent ugly cluttering)

Checklist: ☑️

Updated the documentation for feature additions and enhancements
Added tests for bug fixes or features
Linked all related issues
No unrelated changes

* Add caret icon (as most minimised panels tend to use a caret icon to show that it is a clickable button * Add top margin (as expanded cards also have a top margin but not a bottom margin, top margin was chosen. This is to prevent ugly cluttering) Note: I think for code maintenance, we could probably remove some of the styles in NestedPanel.vue . If I remember correctly, we only need to specify the styles to change in the responsive breakpoints, so is it really needed to specify the same style twice? Note 2: https://developer.mozilla.org/en-US/docs/Web/CSS/float suggests that it might be unneeded to specify display: inline-block if we are also specifying float, as it would be overridden and ignored.

damithc · 2021-12-18T15:14:19Z

@kaixin-hc if this is a visual change, please post before-and-after screenshots too

kaixin-hc · 2021-12-18T15:59:26Z

@kaixin-hc if this is a visual change, please post before-and-after screenshots too

Okay! I've edited the first message so that it is more visible.

jonahtanjz

Thanks @kaixin-hc for working on this :) I think putting them on the same line works as it maintains the minimalistic form and at the same time inform the user that the button can be expanded 👍 I am fine with the current design. Prof @damithc @ang-zeyu any thoughts on this?

Just a quick review and left some comments.

I think on mobile if the title of the panel is long, the icon and the text are on different lines which increases the height of the button.

I think it's better to force the icon to be on the same line and then wrap the title if it overflows.

Something like this:

jonahtanjz · 2021-12-19T13:34:57Z

packages/vue-components/src/panels/MinimalPanel.vue

        <slot name="_alt">
-          <slot name="header"></slot>
+          <span :class="['card-title']">


Suggested change

<span :class="['card-title']">

<span class="card-title">

Just a trivial suggestion: If there is only one class, would directly assigning the class be better? Same for the span in NestedPanel

jonahtanjz · 2021-12-19T13:50:20Z

packages/vue-components/src/panels/NestedPanel.vue

+            :class="['glyphicon', 'glyphicon-chevron-right']"
+          ></span>
+        </div>
+        <span :class="['card-title']">


The text for each panel is slightly shifted downwards and no longer centered vertically.

Seems like the vertical-align: middle; from .card-title is causing it to move downwards.

damithc · 2021-12-19T14:22:39Z

I think putting them on the same line works as it maintains the minimalistic form and at the same time inform the user that the button can be expanded 👍 I am fine with the current design. Prof @damithc @ang-zeyu any thoughts on this?

I'm OK with this design 👍

ang-zeyu · 2021-12-23T06:19:00Z

ok with the design as well! A caret icon seems like a good balance.

Could be expanded by allowing the user to turn off the icon the same way they can turn off the buttons for the panels

Agreed, let's add this in, something like no-minimized-switch (can be less verbose)

Another (subjective) suggestion:

round the buttons into pills to make it a little sleeker, so it blends in better with content .

damithc · 2021-12-23T06:25:39Z

round the buttons into pills to make it a little sleeker, so it blends in better with content .

Yup, this blends in better with the content. The downside is that a minimized panel 'changes shape' when expanded as panels are actually square-shaped, causing a bit of a disconnect between the minimal and the other forms. So, I'm on the fence about this one.

ang-zeyu · 2021-12-23T06:54:46Z

Yup, this blends in better with the content. The downside is that a minimized panel 'changes shape' when expanded as panels are actually square-shaped, causing a bit of a disconnect between the minimal and the other forms. So, I'm on the fence about this one.

Preview:

It does seem a little disconnected, but I'm personally leaning more toward rounding the buttons for a sleeker look with the rest of the page.

I think its something we can eventually "smooth out" to some degree by adding some transitions between the minimised and expanded form as well. (or we could round the existing panels a tad bit more)

kaixin-hc · 2021-12-23T07:30:46Z

Thanks for the review and comments everyone. The preview makes it really easy to visualise. I do think that animations will help to smooth it out, and while I don't know how to apply animations yet it might be nice even if they are not rounded. I'll look into it in the new year!

I'm not sure about fully rounding the corners though, I think it might depend on if users tend to put many minimised expandable panels near each other. I found this article https://uxdesign.cc/make-sense-of-rounded-corners-on-buttons-dfc8e13ea7f7 which made a case for fully rounded buttons for primary content when you have space to spare, to direct users attention to those buttons. They suggested to avoid fully rounded buttons when many are used next to each other as it may not be obvious which to click.

Since I assume the minimised form of the expandable panels would be used more when there are many expandable panels next to each other, I wonder if the eye might become more drawn to the round buttons.

ang-zeyu · 2021-12-23T09:45:56Z

Thanks for sharing the article; That was a good read.

I think it might depend on if users tend to put many minimised expandable panels near each other

I agree fully rounded buttons wouldn't be too suitable here, it might be confused for multiple selection type interfaces.

Not sure if this is a common use case as well @damithc

We could also experiment with slightly milder settings and see which works best (20px -> 10px), I would also be fine if we delayed this decision to some later time completely =P

I do think that animations will help to smooth it out, and while I don't know how to apply animations yet it might be nice even if they are not rounded. I'll look into it in the new year!

👍

can be done separately too, and might even be a little out of scope here as well

Some more screenshots for comparison:

semi rounded 10px (fully rounded is 20px)

fully rounded (original here):

damithc · 2022-01-04T09:47:55Z

I agree fully rounded buttons wouldn't be too suitable here, it might be confused for multiple selection type interfaces.

Not sure if this is a common use case as well @damithc

In the CS2103 website (and its variants), we do have such cases. So, I'm leaning slightly away from fully-rounded corners in this case.

ang-zeyu · 2022-01-14T14:15:27Z

@kaixin-hc

Agreed, let's add this in, something like no-minimized-switch (can be less verbose)

just in case you missed this ^

As for the corner rounding, you could try a couple of settings and post back here; Also ok if it turns out the best setting is the current / we leave it be for now.

kaixin-hc · 2022-01-23T08:06:16Z

I tried a couple of settings for corner rounding; it looked similar to what you posted previously. However, I still feel looking at it that the original settings are best for visual consistency with the rest of MarkBind as it matches the rounding of other components. The inconsistency makes it look a little jarring to me (and again, a bit like tags!)

The current appearance is as follows:

jonahtanjz

Nice work @kaixin-hc 👍 A couple of suggestions on the implementation.

I tried a couple of settings for corner rounding; it looked similar to what you posted previously. However, I still feel looking at it that the original settings are best for visual consistency with the rest of MarkBind as it matches the rounding of other components. The inconsistency makes it look a little jarring to me (and again, a bit like tags!)

We can leave the corner rounding as it is for now. We can come back to this again or as a separate issue, round the corners of the other components as well if it makes the entire site "sleeker".

jonahtanjz · 2022-01-24T08:18:51Z

docs/userGuide/syntax/panels.mbdf

@@ -104,11 +104,14 @@
 </include>


-**Show/Hide buttons using `no-switch` or `no-close`.**
+**Show/Hide buttons using `no-switch`, `no-close`, or `no-mimized-switch`.**


Suggested change

**Show/Hide buttons using `no-switch`, `no-close`, or `no-mimized-switch`.**

**Show/Hide buttons using `no-switch`, `no-close`, or `no-minimized-switch`.**

Typo here :)

jonahtanjz · 2022-01-24T08:18:55Z

packages/vue-components/src/panels/NestedPanel.vue

+    .minimal-caret-wrapper {
+        display: inline-block;
+        font-size: 13px;
+        margin-right: 2px;


Might want to slightly increase the margin between the minimized switch and the card title on mobile? Right now it looks a little too cluttered.

jonahtanjz · 2022-01-24T08:26:48Z

packages/vue-components/src/panels/NestedPanel.vue

@@ -226,8 +236,10 @@ export default {
    .card-title {
        display: inline-block;
        font-size: 1em;
+        line-height: 1.2em;


Just an opinion:

I think the card titles look slightly more centered vertically without setting the line-height here.

With line-height:

Without line-height:

I see what you mean, but it helps for panels with long names.

With line-height:

Without line-height:

Ahh alright then can keep it as it is for now :)

jonahtanjz · 2022-01-24T08:30:29Z

packages/vue-components/src/panels/NestedPanel.vue

+    .glyphicon {
+        display: inline-block;
+    }


I believe the provided css from bootstrap-glyphicons already sets the display to inline-block so should be able to remove this.

kaixin-hc · 2022-02-07T03:53:03Z

Thanks for the review @jonahtanjz, sorry for the delay in implementing changes. I've done as you suggested for all except line height (screenshot explanation in the comments)

jonahtanjz

Thanks for making the changes @kaixin-hc!

LGTM 👍

jonahtanjz reviewed Dec 19, 2021

View reviewed changes

Center text, wrap text

d0c864a

kaixin-hc added 5 commits January 15, 2022 13:54

kiv

b16f628

Merge branch 'master' into minimized-panel-UI

625ca82

Fix centring, toggle caret

34b7e83

Change to no-minimized-switch, update docs

3578e5a

Fix lint

0020578

jonahtanjz reviewed Jan 24, 2022

View reviewed changes

kaixin-hc added 2 commits February 7, 2022 11:39

Merge branch 'master' into minimized-panel-UI

b2a3d16

Fix typo + improve code quality

b435cc3

jonahtanjz approved these changes Feb 7, 2022

View reviewed changes

jonahtanjz added this to the 4.0 milestone Feb 7, 2022

jonahtanjz merged commit 40bb7a5 into MarkBind:master Feb 8, 2022

ong6 mentioned this pull request Feb 13, 2022

Improve minimal panel design #1422

Closed

ang-zeyu mentioned this pull request Jul 31, 2022

Minimal panel button wrapper is on the next line #1979

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enhance appearance of minimised panels #1694

Enhance appearance of minimised panels #1694

kaixin-hc commented Dec 18, 2021 •

edited

Loading

damithc commented Dec 18, 2021

kaixin-hc commented Dec 18, 2021

jonahtanjz left a comment •

edited

Loading

jonahtanjz Dec 19, 2021

jonahtanjz Dec 19, 2021

damithc commented Dec 19, 2021

ang-zeyu commented Dec 23, 2021

damithc commented Dec 23, 2021

ang-zeyu commented Dec 23, 2021

kaixin-hc commented Dec 23, 2021

ang-zeyu commented Dec 23, 2021

damithc commented Jan 4, 2022 •

edited

Loading

ang-zeyu commented Jan 14, 2022

kaixin-hc commented Jan 23, 2022

jonahtanjz left a comment •

edited

Loading

jonahtanjz Jan 24, 2022

jonahtanjz Jan 24, 2022

jonahtanjz Jan 24, 2022

kaixin-hc Feb 7, 2022

jonahtanjz Feb 7, 2022

jonahtanjz Jan 24, 2022 •

edited

Loading

kaixin-hc commented Feb 7, 2022

jonahtanjz left a comment

	Show/Hide buttons using `no-switch`, `no-close`, or `no-mimized-switch`.
	Show/Hide buttons using `no-switch`, `no-close`, or `no-minimized-switch`.

Enhance appearance of minimised panels #1694

Enhance appearance of minimised panels #1694

Conversation

kaixin-hc commented Dec 18, 2021 • edited Loading

damithc commented Dec 18, 2021

kaixin-hc commented Dec 18, 2021

jonahtanjz left a comment • edited Loading

Choose a reason for hiding this comment

jonahtanjz Dec 19, 2021

Choose a reason for hiding this comment

jonahtanjz Dec 19, 2021

Choose a reason for hiding this comment

damithc commented Dec 19, 2021

ang-zeyu commented Dec 23, 2021

damithc commented Dec 23, 2021

ang-zeyu commented Dec 23, 2021

kaixin-hc commented Dec 23, 2021

ang-zeyu commented Dec 23, 2021

damithc commented Jan 4, 2022 • edited Loading

ang-zeyu commented Jan 14, 2022

kaixin-hc commented Jan 23, 2022

jonahtanjz left a comment • edited Loading

Choose a reason for hiding this comment

jonahtanjz Jan 24, 2022

Choose a reason for hiding this comment

jonahtanjz Jan 24, 2022

Choose a reason for hiding this comment

jonahtanjz Jan 24, 2022

Choose a reason for hiding this comment

kaixin-hc Feb 7, 2022

Choose a reason for hiding this comment

jonahtanjz Feb 7, 2022

Choose a reason for hiding this comment

jonahtanjz Jan 24, 2022 • edited Loading

Choose a reason for hiding this comment

kaixin-hc commented Feb 7, 2022

jonahtanjz left a comment

Choose a reason for hiding this comment

kaixin-hc commented Dec 18, 2021 •

edited

Loading

jonahtanjz left a comment •

edited

Loading

damithc commented Jan 4, 2022 •

edited

Loading

jonahtanjz left a comment •

edited

Loading

jonahtanjz Jan 24, 2022 •

edited

Loading