Skip to content
This repository has been archived by the owner on Oct 23, 2023. It is now read-only.

feat: restore slot handling based on element contents #548

Merged
merged 9 commits into from
Jun 13, 2018

Conversation

marionebl
Copy link
Contributor

@marionebl marionebl commented Jun 10, 2018

@tilmx: Could you improve the styling of components/element-slot? Thanks!

@tilmx
Copy link
Member

tilmx commented Jun 10, 2018

Really cool! Thanks for restoring that feature!

I just came across three thingies:

  • Can we parse the @name Lorem ipsum jsdoc annotation as slot label? At the moment only the technical name is used.
  • Can we make the entire element collapsable? At the moment if a element contains regular children and slots, only the children get collapsed when clicking the arrow in the element tree (the slots stay there).
  • When I drag an element into the whitespace below the element tree, nothing happens. Before, the element has been added as last child of the root.

That would be great!

props.element.getNameEditable();
props.element.getHighlighted();
props.element.acceptsChildren();
// props.element.getSelected();
Copy link
Member

Choose a reason for hiding this comment

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

Do we need that code here?

@marionebl marionebl force-pushed the feat/restore-slot-in-elements branch from 61c6204 to c411db0 Compare June 10, 2018 18:25
display: flex;
align-items: center;
color: ${Color.Grey20};
position: relative;
Copy link
Member

Choose a reason for hiding this comment

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

This is a duplicate to line 60

position: relative;
display: flex;
align-items: center;
color: ${Color.Grey20};
Copy link
Member

Choose a reason for hiding this comment

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

in line 68 this is overridden

box-sizing: border-box;
margin-left: ${getSpace(SpaceSize.XXL) - 3}px;
overflow: hidden;
padding: ${getSpace(SpaceSize.XS)}px ${getSpace(SpaceSize.XXS)}px ${getSpace(SpaceSize.XS)}px
Copy link
Member

Choose a reason for hiding this comment

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

this could be written shorter as padding: ${getSpace(SpaceSize.XS)}px ${getSpace(SpaceSize.XXS)}px;

position: relative;
display: flex;
align-items: center;
color: ${Color.Grey20};
Copy link
Member

Choose a reason for hiding this comment

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

this can be removed. It is already overridden in line 96

@tilmx
Copy link
Member

tilmx commented Jun 12, 2018

@lkuechler Good spots – I removed the duplicates!

# Conflicts:
#	src/components/element/demo.tsx
#	src/components/element/index.tsx
#	src/components/index.ts
#	src/container/element-list/element-container.tsx
@tilmx tilmx dismissed lkuechler’s stale review June 13, 2018 20:09

Fixed the stuff!

@tilmx tilmx merged commit 56a60cd into master Jun 13, 2018
@tilmx tilmx deleted the feat/restore-slot-in-elements branch June 13, 2018 20:10
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