-
Notifications
You must be signed in to change notification settings - Fork 224
feat: update styling of layout and element pane #393
Conversation
src/lsg/patterns/element/index.tsx
Outdated
|
||
&::before { | ||
background: ${colors.grey90.toString()}; | ||
} | ||
` | ||
: ''}; | ||
`; | ||
|
||
const StyledPlaceholder = styled.div` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@lkuechler Is it possible to show the placeholder (drop area) only when an element is being dragged around? So it doesn’t react to clicks?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@marionebl takes care of that
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Solved with 4f6f148
src/lsg/patterns/element/index.tsx
Outdated
position: relative; | ||
z-index: 1; | ||
`; | ||
|
||
const StyledElementLabel = styled.div` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@lkuechler Is it possible to apply styling on the element during dragging around? So like only showing the element title text instead of a screenshot of the whole element?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Solved with 71c298d
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some comments, looks great!
src/component/container/app.tsx
Outdated
> | ||
<ElementPane> | ||
<ElementList /> | ||
<AddButton |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't this be invisible / disabled if the right sidebar displays RightPane.Patterns
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought an active state would be cooler, to not disturb the user because the button is sometimes not there. But let’s test that as well (see below)!
src/component/container/app.tsx
Outdated
onClick={e => { | ||
e.stopPropagation(); | ||
store.setRightPane(RightPane.Patterns); | ||
store.setSelectedElement(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't quite get how the Add Elements
button is different from "unfocussing" the currently selected element if we remove the selection. Would it make more sense to retain the current selection while showing the RightPane.Patterns
panel?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As interacting with the left layers changes the right property pane, the idea was to add a button that shows the pattern view. And the short cut for not having to directly click on the button left bottom would be to click on the unused area in the element view (so it unselects the element). Then you always see on the right side what you want to do (like inspect on macOS does).
I’d love to test this in a kind of usertest, if we find out it doesn’t work – let’s change it.
font-size: 12px; | ||
opacity: 1; | ||
top: 0; | ||
left: -500px; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the purpose of this offset?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Scratch that - I think this should be something like to ensure this is outside the viewport in any case
/* Ensure cloned drag image node is outside viewport */
position: fixed;
top: 100vh;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point. Thanks!
if (dragElement) { | ||
const dragImg = dragElement.cloneNode(true) as HTMLElement; | ||
dragImg.setAttribute('style', DRAG_IMG_STYLE); | ||
document.body.appendChild(dragImg); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We create and never clean up the dragImg nodes here. I think we should save a reference to the dragImg node on the instance and cleanup in handleDragEnd
:
private handleDragEnd(e: React.DragEvent<HTMLElement>): void {
this.setState({ dragging: false });
if (this.dragImg) {
this.dragImg.parentNode.removeChild(this.dragImg);
}
}
private handleDragStart(e: React.DragEvent<HTMLElement>): void {
/* code emitted */
const dragImg = dragElement.cloneNode(true) as HTMLElement;
document.body.appendChild(dragImg);
this.dragImg = dragImg;
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I get the idea, but what do mean with "this.dragImg = dragImg;" instead of "e.dataTransfer.setDragImage(dragImg, 75, 15);"? Now it doesn’t set the drag image anymore. Or did you mean to use "this.dragImg" inside "setDragImage" then?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I omitted the e.dataTransfer.setDragImage(dragImg, 75, 15);
statement, that's still needed. this.dragImg
should happen right after e.dataTransfer.setDragImage(dragImg, 75, 15);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Solved with 70d4708
src/electron/main.ts
Outdated
@@ -17,6 +17,7 @@ function createWindow(): void { | |||
minWidth: 780, | |||
minHeight: 380, | |||
titleBarStyle: 'hiddenInset', | |||
backgroundColor: '#f7f7f7', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This color should be registered with lsg/patterns/colors/index.tsx
and moved from there. Also use it in global styles: https://github.com/meetalva/alva/blob/master/src/lsg/patterns/global-styles/index.tsx#L9
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch. In "global-styles" that makes sense – but as an open question: Wouldn’t it be better to directly set the color here in main.ts in terms of startup speed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, nothing wrong with applying a background color here, but I'd recommend we the actual value from the colors pattern.
} | ||
|
||
const StyledAddButton = styled.div` | ||
position: absolute; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch, thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As position: fixed
would have caused that the button can’t have the same width as the parent, I restyled the preview pane with display flex vertically (+ overflow: scroll in the element section).
src/lsg/patterns/chrome/index.tsx
Outdated
padding: ${getSpace(SpaceSize.XS)}px ${getSpace(SpaceSize.XXL) * 3}px; | ||
border-bottom: 0.5px solid ${colors.grey90.toString()}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should not drop on subpixels - this might produce undesirable output on devices with lower pixel density.
This should be guarded with a media query, like this:
border-bottom-width: 1px;
@media screen and (-webkit-min-device-pixel-ratio: 2) {
border-bottom-width: 0.5px;
}
src/lsg/patterns/element/index.tsx
Outdated
padding: ${getSpace(Size.XS)}px; | ||
transition: transform 0.2s; | ||
|
||
${(props: StyledIconProps) => (props.open ? 'transform: rotate(90deg)' : '')}; | ||
${(props: StyledIconProps) => (props.active ? 'fill: white' : '')}; | ||
${(props: StyledIconProps) => (props.active ? 'fill: #0070D6' : '')}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be deduced from styleguide colors
src/lsg/patterns/list/index.tsx
Outdated
props: StyledListItemProps | ||
) => (props.active ? 'background: #def' : '')}; | ||
${(props: StyledListItemProps) => (props.onClick ? 'cursor: pointer;' : '')}; | ||
${(props: StyledListItemProps) => (props.active ? 'background: #def' : '')}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Move the colors pattern
* feat(component): update styling of layout * feat(component): update styling of element pane * feat: unselect items when sidebar root receives click * refactor: simplify app view by moving out splashscreen * style: use tabs for indentation * feat(component): add drag image styling * fixup! increase element drag size * fixup! add check for null on drag element * fix: avoid side effect froms drop areas on click interaction * fix: remove obsolete handler * fix: select right drag element target * fix: adapt to new interface * fix: make ts happy * fix: restore icon click functionality * feat: show either properties or patterns pane * fix: restore pattern drag * fix: set backgroundColor during startup * fix: handle draggable states via unified state * feat: add button to show pattern pane * feat: add button styling * fixup! change requests * fix: pass hex color to BrowserWindow * refactor: use 100vh to move element outside of viewport * fix: clean up dragimg nodes after dragging * fix: get color from styleguide * fix: read drag image colors from styleguide * fix: update add button position and side bar borders * fix: set hover and active styling to preview resizer * fix: add element overflow gradient * fix: make entire tree element draggable * fix: reenable placeholder targets for off-list payloads * fix: do not write bogus dom attributes
Bringing the chrome, preview and element pane to the freshest style version!
The pull request will be squashed into "feat: update styling of layout and element pane"