Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

collapsible component lists #1324

Merged
merged 4 commits into from Sep 7, 2018
Merged

collapsible component lists #1324

merged 4 commits into from Sep 7, 2018

Conversation

nelsonpecora
Copy link
Contributor

@nelsonpecora nelsonpecora commented Sep 5, 2018 •
edited

  • add collapse: true to _componentList config to enable
  • will always display a placeholder with the placeholder text
  • allows hiding/showing of list children

Empty List:

list_empty

Open List:

list_open

Collapsed List:

list_collapsed

rmfarrell
rmfarrell suggested changes Sep 5, 2018
View reviewed changes
Copy link
Contributor

@rmfarrell rmfarrell left a comment

Choose a reason for hiding this comment

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

The collapse expand interaction looks really good! A couple questions about the internals

docs/manipulating-components.md Outdated
@@ -46,6 +46,30 @@ myList:

![](images/list_placeholder.png)

#### Collapsible Lists

Basic lists may also be collapsed. This is useful if your components aren't a 1:1 WYSIWYG match with their public-facing view, for example if you have components that only display in `edit` mode.
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this needs a better explanation. Can I suggest:
"In edit mode, component lists can can be expanded and collapsed.This feature affects edit mode only, however, so it's most useful for controlling non-visible component lists to reduce clutter for the editor."

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I like it!

lib/decorators/placeholder.vue Outdated
.collapsible-component-list > .kiln-placeholder {
height: 68px !important;
margin: 0 0 20px;
min-height: 68px !important;
Copy link
Contributor

Choose a reason for hiding this comment

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

!important is an anti-pattern

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I should add a comment here. Basically this is overriding the normal (inline style attr) height stuff.

lib/decorators/placeholder.vue Outdated
<div class="ui-button-group">
<ui-button v-if="isEmptyList" :disabled="!isActive" class="placeholder-add-component" icon="add" :color="placeholderButtonColor" @click.stop.prevent="openAddComponentPane">{{ addComponentText }}</ui-button>
<ui-icon-button v-else class="placeholder-collapse-button" :icon="collapseIcon" :tooltip="collapseTooltip" :color="placeholderButtonColor" @click.stop.prevent="toggleCollapse"></ui-icon-button>
</div>
Copy link
Contributor

Choose a reason for hiding this comment

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

is this wrapper necessary? it looks like only one of these buttons appears at at a time

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah yeah, I can remove it. Originally I was going to show both, but we should show the Add Component button only when it's empty. 👍

lib/decorators/placeholder.vue Outdated
<ui-icon-button v-else class="placeholder-collapse-button" :icon="collapseIcon" :tooltip="collapseTooltip" :color="placeholderButtonColor" @click.stop.prevent="toggleCollapse"></ui-icon-button>
</div>
</div>
<ui-button v-else-if="isComponent" :disabled="!isActive" class="placeholder-add-component" icon="add" :color="placeholderButtonColor" @click.stop.prevent="openAddComponentPane">{{ addComponentText }}</ui-button>
Copy link
Contributor

Choose a reason for hiding this comment

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

The big v-ifs here have a whiff of code some smell to it. I think this can be made DRYer either by using slots or writing conditions before each individual item is added to the markup. If the types of placeholders are really so different it should by another component by Singe Responsibility (tho I think that's not the case here).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, I don't think having the three conditions written out with v-ifs is terrible, but I'm going to add comments to them (which will also be helpful if we end up destructuring this code in the future if placeholders become too complex)

lib/decorators/placeholder.vue Outdated
this.isCollapsed = true;
this.$el.parentNode.classList.add('kiln-collapsed');
}
},
Copy link
Contributor

Choose a reason for hiding this comment

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

Would this work here?

this.isCollapsed = !this.isCollapsed
// ...add/remove conditional here or classList.toggle()

More importantly, is there a more Vue-friendly way to do this than external DOM manipulation? Seems fragile

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Haha yeah I could just do that 🙃 (I forgot about classList.toggle())

As for the fragility, I'll add a check to make it less brittle but there isn't really a more vue-y way to do this (since it's reaching out to a non-vue thing)

Copy link
Contributor

Choose a reason for hiding this comment

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

it looks like you're passing the parentHeight to $options. Would it be useful to instead pass the whole parent reference? Esp if the height ever changes it would be better to track that within Vue anyway.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The parent reference might change on re-render

Copy link
Contributor

Choose a reason for hiding this comment

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

oh dang. and the Vue component can't be notified?

@rmfarrell rmfarrell mentioned this pull request Sep 6, 2018
Closed
rmfarrell
rmfarrell approved these changes Sep 7, 2018
View reviewed changes
lib/decorators/placeholder.vue Outdated
@@ -164,7 +195,9 @@

export default {
data() {
return {};
return {
isCollapsed: false
Copy link
Contributor

Choose a reason for hiding this comment

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

isn't this true initially?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was thinking about that, and I think I'd be fine with having it collapsed initially.

Copy link
Contributor

Choose a reason for hiding this comment

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

u mean expanded? !isCollapsed = expanded, no?

@nelsonpecora nelsonpecora merged commit d809ddb into master Sep 7, 2018
@nelsonpecora nelsonpecora deleted the list-collapse branch September 7, 2018 15:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rmfarrell rmfarrell rmfarrell approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@nelsonpecora @rmfarrell