[design] Rework #231

kristofferjs · 2017-10-05T10:40:38Z

A visual cleanup for a better UI on sanity

default-layout

dark tool-selector on the left side.
search on top

desk-tool

brighter content-type-pane with shadow in desk-tool
consistent styling of content-types-pane
Consistent styling of editor top (until we have a better solution)

components

A StickyPortal component that follow the scrollcontainer with an onResize-prop
Popover that uses StickyPortal to place itself
DropDownButton that uses StickyPortal to place itself
SearchableSelect that uses StickyPortal to place itself

form-builder

Better handling of overflowing toolbar in block-editor (needs collapsing-feature later)
Nicer full view
more margin on array lists

preview

always assume image/media until we can determine if it should be visible or not

bjoerge

This is great work, and the new design looks awesome 😍

I've done a quick code review, and got a few comments. Will do more testing tomorrow.

bjoerge · 2017-10-05T17:07:15Z

packages/@sanity/desk-tool/src/pane/styles/contentStylesOverride.css

+@import 'part:@sanity/base/theme/variables-style';
+
+.root {
+  composes: root from '../../../../components/src/panes/styles/DefaultPane.css';


This doesn't seem quite right? Should it have been imported as a part instead?

Yes. It seems wrong. But exposing a part for this is also wrong. Im not comfortable exposing all CSS-parts without a documentation. I think all composing internally should be done like this, unless it is a documented part.

But this includes it from outside of the package, and I'm 99% sure this will break in a standalone studio (where this code resides in node_modules). So either this has to be required from the @sanity/components module, or from a part.

I understand. I will make a part of this.

bjoerge · 2017-10-05T17:07:25Z

packages/@sanity/desk-tool/src/pane/styles/contentStylesOverride.css

+}
+
+.headerBackground {
+  composes: headerBackground from '../../../../components/src/panes/styles/DefaultPane.css';


bjoerge · 2017-10-05T17:09:23Z

packages/@sanity/imagetool/src/HotspotImage.js

@@ -142,7 +142,7 @@ export default class HotspotImage extends React.PureComponent {
          <div style={targetStyles.crop}>
            <img
              ref={this.setImageElement}
-              src={src}
+              src={`${src}?w=500&fit=max`}


Same as the above comment.

bjoerge · 2017-10-05T17:10:30Z

packages/@sanity/form-builder/src/inputs/Image/ImageInput.js

-          <ImageTool value={{hotspot, crop}} src={imageUrl} onChange={this.handleImageToolChange} />
+          <ImageTool
+            value={{hotspot, crop}}
+            src={`${imageUrl}?w=500&fit=max`}


This will only work if the image url is behind a CDN that supports these params. Can you add these in another layer perhaps?

Agreed. We need a solution for this. Also maybe out of the scope of this pull request.

bjoerge · 2017-10-05T17:14:29Z

packages/@sanity/components/src/utilities/tryFindScrollContainer.js

@@ -0,0 +1,31 @@
+/* global window, document */
+export default function tryFindScrollContainer(element, callback) {


Why a callback here? It doesn't do anything async?

bjoerge · 2017-10-05T17:18:59Z

packages/@sanity/components/src/utilities/tryFindScrollContainer.js

+    scrollContainer = document.body
+  }
+
+  return callback(scrollContainer)


I think its better if you just return the scrollContainer here, and where you use it, you can pass it directly to the callback, like this:

callback(tryFindScrollContainer(element))

// e.g instead of. tryFindScrollContainer(element, this.setScrollContainerElement) // you can just do this.setScrollContainerElement(tryFindScrollContainer(element))

bjoerge · 2017-10-05T17:19:31Z

packages/@sanity/components/src/tags/styles/TextField.css

@@ -85,9 +85,9 @@
  composes: focusHelper from 'part:@sanity/base/theme/forms/text-input-style';
  background-color: transparent;

-  @nest .hasFocus & {
+  /*@nest .hasFocus & {


Remove this?

bjoerge · 2017-10-05T17:20:27Z

packages/@sanity/components/src/previews/DefaultPreview.js

@@ -38,6 +38,10 @@ class DefaultPreview extends React.Component {
    item: {}
  }

+  shouldComponentUpdate() {
+    return true


Why do we need this?

Old stuff from before PureComponent that the linter complained about. Will remove.

bjoerge · 2017-10-05T17:21:53Z

packages/@sanity/components/src/edititem/PopOver.js

    popOverStack.pop()
    const prevPopOver = popOverStack.slice(-1)[0]
    if (prevPopOver) {
      setFocus(prevPopOver)
-      prevPopOver.moveIntoPosition()
+      // prevPopOver.moveIntoPosition()


remove this?

bjoerge · 2017-10-05T17:30:30Z

packages/@sanity/components/src/edititem/PopOver.js

+      <div style={{display: 'span'}} ref={this.setRootElement}>
+        <StickyPortal
+          isOpen={isOpen}
+          scrollContainer={scrollContainer}


Why do we need to pass this down? If StickyPortal needs some values from the scrollContainer, its a lot better to pass down these values instead.

It needs to add an eventListener and get the positions. I think that an ref to an element is a good way to go.

I dont like the "tryFindScrollContainer" way of doing this. In the future it would be nice to control this by passing down props from where the scrollContainer is (desk-tool in this case), or have an global system to control modals/overlay. This is also good for determine what should be closed on pressing escape etc.

Yeah, lets try to fix this properly later.

bjoerge · 2017-10-09T15:50:57Z

packages/@sanity/base/src/styles/forms/text-input.css

@@ -31,18 +33,20 @@
 }

 .focusHelper {
+  display: none;


Looks like this should not be here? It's overridden on the line below.

bjoerge · 2017-10-09T15:51:22Z

packages/@sanity/base/src/styles/forms/text-input.css

  background-color: transparent;
 }

 .focusHelperActive {
  background-color: var(--focus-color);
+  background-color: transparent;


This is also duplicated.

bjoerge · 2017-10-09T15:58:09Z

packages/@sanity/components/src/textinputs/styles/DefaultTextInput.css

@@ -26,15 +26,15 @@
  composes: focusHelper from 'part:@sanity/base/theme/forms/text-input-style';
  background-color: transparent;

-  @nest .hasFocus & {
+  /*@nest .hasFocus & {


Can you remove this?

bjoerge · 2017-10-09T16:03:46Z

packages/@sanity/desk-tool/src/pane/TypePane.js

@@ -33,7 +34,7 @@ export default withRouterHOC(class TypePane extends React.PureComponent {
    // const isActive = !selectedType && !action && !selectedDocumentId

    return (
-      <Pane {...this.props}>
+      <Pane {...this.props} styles={contentStylesOverride}>


Is this intentional? Isn't contentStylesOverride a CSS modules-export - shouldn't it be a selector and passed to classNames instead of styles? Or am I missing something?

It is intentional, because Pane is a styleable component.

Of course. Got confused by the prop-name 🙃 .

bjoerge · 2017-10-09T16:05:44Z

packages/@sanity/form-builder/src/inputs/Array/ItemValue.js

@@ -107,7 +107,7 @@ export default class Item extends React.Component<Props> {

    return (
      <div className={styles.popupAnchor}>
-        <EditItemPopOver title={memberType.title} onClose={this.handleEditStop}>
+        <EditItemPopOver onClose={this.handleEditStop} key={`popOver${item._key}`}>


No need to prefix the key here as long as it is unique within its parent, so its cleaner to just do key={item._key} here :)

rexxars · 2017-10-09T17:18:47Z

I personally find the dividers here a bit off-putting.. Could they perhaps be slightly weakened in color? Also, perhaps hide border-top on the first element and border-bottom on the last element?

rexxars · 2017-10-09T17:20:40Z

The top-left title is not very readable ;)
Do we have a plan with this area? Hard to fit a longer title in there, perhaps?

rexxars · 2017-10-09T17:23:19Z

The "select from library" dialog loads 100x100px images but have a container that is smaller. This results in overlapping images and weird hover-states.

rexxars · 2017-10-09T17:23:47Z

That's the only design issues I've been able to find! Great work! 👏

bjoerge · 2017-10-09T21:29:20Z

Looks like the reference select doesn't capture clicks anymore:

Edit: and using arrow keys to navigate in the list doesn't highlight items either.

annoyances

…ral cleanup

…metimes

…ge-tool fields

bjoerge · 2017-10-11T17:15:51Z

I think this is finally good to merge now. There's a few remaining minor issues that I'll create separate tickets for. So lets merge this into next.

@kristofferj Would you like to hit the Squash and merge button?

* [components] Adding first version of sticky portal component * [design] Testing new layout * [design] Adjusting input components * [design] Adjusting layout * [design] Tweaking preview and list * [components] Fix button margin * [design] Global selectable states * [design] White content pane * [components] Making menu simpler and using portal-component and tweaking animations * [design] Tweaks on colors etc * [form-builder] Hotspot not loading big images * [components] Tweaks on scrolling * [components] Tidy up the scrolling and overlay * [form-builder] Improving the UI on block editor * [components] Pane content top fix * [components] Pane cleanup and scroll shadow * [form-builder] Linting CSS * [base] Adding check and sync icon * [desk-tool] Styling status in Editor * [default-layout] Fixing top default lauout and search * [default-layout] Fix search * [components] NoMedia on mediarender supports small sizes * [base] Adding back backgrounds style part * [date-input] Using React-datepicker * [components] Fixing dropdown button * [components] Fixing fold out * [componets] Small UI tweaks * [components] Using StickyPortal for dropdown on searchable select * [components] Linting some files * [components] Support origin left right on dropdown button * [desk-tool] Passing up scroll from VirtualList to Pane * [components] Removing callback from tryFindScrollContainer * [pr] Minor changes from pull request * [form-builder] Adding movingItemClass to array of primitives * [components] Progress on preview * [components] Making default pane styles as a part * [base] Overlay on listItemStates * [default-lauout] Better toolSwitcher design * [components] Using CSS grid layout on grid list * [default-layout] Search design improvements * [components] No shadow on pane header when collapsed * [components] Support for title on popover * [components] Fix style on defaultPane * [form-builder] Fixing inline block element * [form-builder] Remove console log * [components] Using lists the intended way * [default-layout] Mobile support for launch design * [form-builder] Fixing styling on array of primitives * [default-layout] Fix branding on desktop] * [default-layout] Mobile header fix * [components] Fix popover for mobile * [form-builder] Fix grid list * [form-builder] Actually use dragHandle when in use * [design] Cleaning up small design annoyances * [form-builder] Making sortable array not show itemStates when drag/movin * [components] SearchableSelect. Not close on click in sticky, and general cleanup * [default-layout] Handling of branding in new design * [form-builder] Clean up reference input and fix a few minor issues * [css] Remove backdrop-filter * [form-builder] Styling of selectAsset * [date-input] Fixing year and month dropdown * [components] Fix: reference picker was emitting an undefined value sometimes * [default-layout] Fix firefox bug * [default-layout] Dont show projectName when brandingLogo exists * [default-layout] Exclude sanity-prefixed types from search * [form-builder] Don't do any conversion on value given to number input * [form-builder] Only display hotspot heading if there are actually image-tool fields * [form-builder] Remove width constraints for now

bjoerge self-requested a review October 5, 2017 10:42

kristofferjs changed the base branch from master to next October 5, 2017 10:43

bjoerge requested changes Oct 5, 2017

View reviewed changes

kristofferjs force-pushed the design/launch-design branch from e5a4bcf to c8f1d6c Compare October 6, 2017 09:56

rexxars force-pushed the next branch from 440cc2b to ef4965f Compare October 6, 2017 13:03

kristofferjs force-pushed the design/launch-design branch 4 times, most recently from 2b9582f to 1e8ff1b Compare October 9, 2017 13:58

bjoerge reviewed Oct 9, 2017

View reviewed changes

kristofferjs force-pushed the design/launch-design branch from d0752d3 to fa9884e Compare October 10, 2017 09:29

bjoerge force-pushed the design/launch-design branch from fa9884e to 30c19dc Compare October 10, 2017 10:09

kristofferjs force-pushed the design/launch-design branch 2 times, most recently from 9454f96 to beb8e34 Compare October 10, 2017 18:02

bjoerge force-pushed the design/launch-design branch 3 times, most recently from 9de3d87 to d211825 Compare October 11, 2017 09:38

kristofferjs force-pushed the design/launch-design branch 2 times, most recently from f306795 to 3522168 Compare October 11, 2017 11:02

bjoerge force-pushed the next branch from 7247699 to b72e0eb Compare October 11, 2017 11:10

bjoerge force-pushed the design/launch-design branch from 3522168 to 4a46245 Compare October 11, 2017 11:10

kristofferjs and others added 16 commits October 11, 2017 17:20

[default-layout] Fix branding on desktop]

15ccd3e

[default-layout] Mobile header fix

b37ea9f

[components] Fix popover for mobile

13483b1

[form-builder] Fix grid list

03dc2b1

[form-builder] Actually use dragHandle when in use

2b36ecf

[design] Cleaning up small design

e21dfd4

annoyances

[form-builder] Making sortable array not show itemStates when drag/movin

114d5c3

[components] SearchableSelect. Not close on click in sticky, and gene…

4991d43

…ral cleanup

[default-layout] Handling of branding in new design

e5cdefb

[form-builder] Clean up reference input and fix a few minor issues

f0513d0

[css] Remove backdrop-filter

1472441

[form-builder] Styling of selectAsset

3254516

[date-input] Fixing year and month dropdown

dde6aa2

[components] Fix: reference picker was emitting an undefined value so…

7963fad

…metimes

[default-layout] Fix firefox bug

3f35824

[default-layout] Dont show projectName when brandingLogo exists

706b941

kristofferjs force-pushed the design/launch-design branch from 50d65b5 to 706b941 Compare October 11, 2017 15:20

bjoerge added 4 commits October 11, 2017 17:21

[default-layout] Exclude sanity-prefixed types from search

518031b

[form-builder] Don't do any conversion on value given to number input

5989aac

[form-builder] Only display hotspot heading if there are actually ima…

e3ea823

…ge-tool fields

[form-builder] Remove width constraints for now

e47ccfd

bjoerge changed the title ~~Design/launch design~~ [design] Rework Oct 11, 2017

bjoerge approved these changes Oct 11, 2017

View reviewed changes

kristofferjs merged commit d4d8592 into next Oct 11, 2017

bjoerge deleted the design/launch-design branch October 11, 2017 18:12

		@@ -0,0 +1,31 @@
		/* global window, document */
		export default function tryFindScrollContainer(element, callback) {

[design] Rework #231

[design] Rework #231

Conversation

kristofferjs commented Oct 5, 2017

bjoerge left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rexxars commented Oct 9, 2017

rexxars commented Oct 9, 2017

rexxars commented Oct 9, 2017

rexxars commented Oct 9, 2017

bjoerge commented Oct 9, 2017 • edited

bjoerge commented Oct 11, 2017 • edited

bjoerge commented Oct 9, 2017 •

edited

bjoerge commented Oct 11, 2017 •

edited