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

Conversation

ang-zeyu
Copy link

@ang-zeyu ang-zeyu commented Dec 16, 2019

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

• [x] Enhancement to an existing feature

Implements MarkBind/markbind#564
Corresponding documentation PR: MarkBind/markbind#960

What is the rationale for this request?
To add minimal ( super-seamless ) type panels, which better conveys that the panel's content is 'optional'

What changes did you make? (Give an overview)
A new type="minimal" value was added to the existing type attribute in the <panel> component,
which allows creating a minimal type panel.

If no header is specified, a ... icon is automatically used. If one is specified, then a down caret icon will be appended to the header, unless the no-switch attribute is specified.

superSeamlessPanel

While expanded, the footer completely disappears from view unless the mouse is hovered over the content or the footer itself. In mobile view, it has a small opacity so that the reader knows that the footer is present.

Provide some example code that this change will affect:

<minimal-panel v-if="isMinimal" v-bind="$attrs" >
    <template v-for="(node, name) in $slots" :slot="name">
        <slot :name="name"></slot>
    </template>
</minimal-panel>

Is there anything you'd like reviewers to focus on?
The design of the minimal panels in general

Testing instructions:
I added a sample usage of minimal type panels to src\template\default\index.md. All the attributes specified in the user guide ( https://markbind.org/userGuide/usingComponents.html#panels ) should produce the expected behaviour, including the usage of custom slot headers.

The appropriate icons should also fade in and out when hovered over, as displayed in the above gif.

Proposed commit message: (wrap lines at 72 characters)
Added minimal type panels

@ang-zeyu ang-zeyu changed the title [WIP] Added minimal type panels [WIP] Add minimal type panels Dec 16, 2019
@damithc
Copy link

damithc commented Dec 17, 2019

Quick comment: try to make the extra decorations subtle (less eye-catching) as much as possible, especially when the panel is expanded. They should not disrupt the visual flow of the page contents.
e.g., use lighter colors, smaller graphics, show only on hover etc.

@ang-zeyu
Copy link
Author

Noted, thanks for the suggestions!

@yamgent yamgent self-requested a review December 17, 2019 05:25
@yamgent
Copy link
Member

yamgent commented Dec 18, 2019

  • Ergo I would like to refactor Panels.vue into one more separate file for the 'normal' and 'seamless' type panels. This would probably result in a cleaner architecture, but some degree of code duplication.

Can MinimalPanel.vue be inheriting from a base PanelBase class instead? Then the normal and seamless ones would also inherit from PanelBase. Then Panel.vue (which is not the base class) would then mainly be selecting the correct type, and passing the properties to the correct type.

@ang-zeyu
Copy link
Author

Can MinimalPanel.vue be inheriting from a base PanelBase class instead?

Good idea, hadn't thought of using mixins. Will try this out.

Thanks!

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.

Good work with the refactoring. I have no comments about the minimal panel implementation currently, I am OK with accepting this current version. So additional stuff to take note:

  • Please add src/panels/PanelSwitch.vue and src/panels/PanelBase.js to .eslintignore.
  • Also remember to open another PR in MarkBind to mention about this feature in the documentation.

@ang-zeyu
Copy link
Author

Thanks for looking through it again!

Will be tweaking and testing it a little more, and making those changes.

@ang-zeyu ang-zeyu force-pushed the minimal-panel branch 2 times, most recently from e5fab07 to c0b3136 Compare December 21, 2019 11:01
@ang-zeyu ang-zeyu changed the title [WIP] Add minimal type panels Add minimal type panels Dec 21, 2019
@ang-zeyu
Copy link
Author

ang-zeyu commented Dec 21, 2019

Made just one extra style change ( the vaguely visible vertical line that appears when you hover over the header ), and cleaned up the refactor in general.

Should be ready for review

@yamgent
Copy link
Member

yamgent commented Dec 23, 2019

Erm the design of the minimal panel seems to have completely changed, probably not intentional?

image

EDIT: Ignore this post, I made a mistake in pulling the branch...

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.

3 participants