[components] Proof of new slug concept #585

kristofferjs · 2018-02-12T08:28:56Z

Not ready to merge

Docs says slugify on options for custom slugify function (code says slugifyFn) Changed to match the docs.
No auto
One button to generate slug based on slugify and source

There are som TODOs in the code now. Biggest issue is that when trying to find an unique slug, it shows validator error between each try.

kristofferjs · 2018-02-14T09:56:30Z

May also be connected to #430

skogsmaskin · 2018-02-15T04:30:35Z

packages/@sanity/form-builder/src/inputs/Slug/SlugInput.js

@@ -129,39 +134,19 @@ export default class SlugInput extends React.Component {
    }
    const {type, slugifyFn} = this.props

-    const slugify = get(type, 'options.slugifyFn') || slugifyFn
+    const slugify = get(type, 'options.slugify') || slugifyFn


This would be a breaking change. Should be kept as is.

ok. need to update the documentation then.

skogsmaskin · 2018-02-15T04:45:27Z

I think we also should fix the issue with invalid slug matching published documents.

The default validate function is here: /packages/@sanity/form-builder/src/sanity/inputs/Slug.js. It should ignore results from a published document that is the same as the draft currently in edit.

kristofferjs · 2018-02-16T13:39:44Z

The validation branch is soon merged. I think we should remove all validatin-stuff from slug, and use the new validation functionality. Will also prevent people from publishing used slugs.

kristofferjs · 2018-02-19T08:41:21Z

@rexxars This is now rebased with next (after validation branch merge).
Just need to do the slug-validation and I think we are good to go. (see slug.js). The query is in Slug.js. I think it should be defined one place. We dont have access to the document-variable now, but I think you had a plan for this.

rexxars · 2018-02-20T09:16:30Z

I've simplified this greatly by removing the factory pattern, as I don't think this is currently in use anywhere. I've also implemented the uniqueness check using the validation pattern and fixed the default uniqueness check to exclude itself in both draft and published form.

Out of date, please re-review

bjoerge

In addition the the comments above, I also found a few issues when testing:

Clicking "Generate Slug" on a new document causes an exception to be thrown (this is related to https://github.com/sanity-io/sanity/pull/585/files#diff-1978e92fdc897387c12581ed8404a418R57 I think)
Doesn't seem like the slug gets updated when clicking "Generate Slug"-button
The slug value gets unset if it has a value, and you click "Generate Slug"-button, also causing a validation error saying Slug is aleady in use
Doesn't seem like we can unset the value? This is maybe not a regression, but should probably be supported?

bjoerge · 2018-02-21T07:46:23Z

packages/@sanity/form-builder/src/inputs/Slug/SlugInput.js

-        return Promise.resolve()
+    slugify(sourceValue) {
+      if (!sourceValue) {
+        return sourceValue


Shouldn't this be wrapped in a Promise.resolve so that this method always returns a promise?

bjoerge · 2018-02-21T07:50:14Z

packages/@sanity/form-builder/src/inputs/Slug/SlugInput.js

-      label: type.title,
-      description: type.description,
-      level: level
+      const newFromSource = typeof source === 'function' ? source(document) : get(document, source)


How do we handle the case where newFromSource may be undefined/empty?

Shouldn't matter, as this.slugify checks for truthyness

bjoerge · 2018-02-21T07:57:09Z

packages/@sanity/components/src/buttons/styles/InInput.css

@@ -32,37 +32,8 @@
  line-height: 1em;
 }



Changes in this file to be unrelated to the slug input. Is it intentional that they are included in this PR?

I'm not sure. @kristofferj ?

bjoerge · 2018-02-21T07:57:34Z

packages/@sanity/components/src/toggles/Switch.js

@@ -13,6 +13,11 @@ export default class Switch extends React.Component {
    readOnly: PropTypes.bool
  }

+  static defaultProps = {


Also seems unrelated to the current PR

bjoerge · 2018-02-21T07:58:58Z

packages/@sanity/components/src/validation/ValidationListItem.js

@@ -9,6 +9,7 @@ export default class ValidationListItem extends React.PureComponent {
    onClick: PropTypes.func,


Unclear to me why changes in this file should be a part of this PR.

bjoerge · 2018-02-21T08:00:23Z

packages/@sanity/form-builder/src/inputs/Slug/SlugInput.js

+          // eslint-disable-next-line no-console
+          console.error(`An error occured while slugifying "${newFromSource}":\n${err.stack}`)
+        })
+        .then(() => this.setState({loading: false}))


Should we guard against the component being unmounted here?

bjoerge · 2018-02-21T08:00:43Z

packages/@sanity/form-builder/src/inputs/Slug/createSlugInput.js

@@ -1,19 +0,0 @@
-import React from 'react'


Good riddance 💃

bjoerge · 2018-02-21T08:09:41Z

packages/@sanity/form-builder/src/inputs/Slug/SlugInput.js

-    this.props.onChange(PatchEvent.from(value ? set(value) : unset()))
-  }
+    updateValue(value) {
+      this.props.onChange(PatchEvent.from(value ? set(value) : unset()))


We should probably make sure we add a type annotation here, and always add _type: this.props.type.name to the value emitted.

rexxars · 2018-02-22T13:25:04Z

Fixed mentioned issues - not sure if the validation list / styling changes were intentional, leaving that up to @kristofferj to decide on

bjoerge · 2018-02-22T15:29:39Z

Nice! Works a lot better now. The only minor issue left is that the behavior when the source field is not set is a bit strange in that:

The generate slug button is still clickable.
When you click it, you actually unset the current value of the slug field.

Would it make sense to disable the "Generate slug" button when the source field is (or the function returns) empty?

rexxars · 2018-02-22T18:11:42Z

Good call. Can't really disable it if the function returns false, surely? But if it's not set, sure.

…ocument

…astructure.

…tate

kristofferjs · 2018-02-28T07:57:36Z

@rexxars @bjoerge This is maybe ready to be merged now?

rexxars · 2018-02-28T08:15:38Z

Still doesn't include a fix for generating a slug until the uniqueness check returns true - if we want to include that in this release, we'll need @bjoerge to expose some way to find the full path of the input from within the input component.

In my opinion this is something we could easily add in the future, though.

bjoerge · 2018-02-28T08:55:25Z

In my opinion this is something we could easily add in the future, though.

Agreed. I created #617 for providing the path to input components.

bjoerge · 2018-02-28T10:33:55Z

@skogsmaskin, @rexxars & @kristofferj Just to be on the safe side: are you all confident that these changes are backward compatible? And that there are no docs that needs to be updated?

rexxars · 2018-02-28T10:42:53Z

The documentation was/is wrong in regards to the slugify/slugifyFn option. I will send a PR that ensures that either one will work and warn when using the slugifyFn-form.

The documentation also needs to be updated to describe the current uniqueness check constraints and how to provide your own. I will update the documentation and make it ready to be published.

kristofferjs requested review from skogsmaskin and bjoerge February 12, 2018 08:28

kristofferjs added the No docs needed label Feb 12, 2018

skogsmaskin previously requested changes Feb 15, 2018

View reviewed changes

bjoerge force-pushed the next branch from d41d8b0 to 292ed3b Compare February 16, 2018 12:16

rexxars force-pushed the next branch from 1460550 to 22807f8 Compare February 16, 2018 16:56

kristofferjs force-pushed the slug-bugs branch from 09538df to 80404f0 Compare February 19, 2018 08:37

rexxars force-pushed the slug-bugs branch from 80404f0 to 98de16d Compare February 19, 2018 11:45

rexxars requested a review from skogsmaskin February 20, 2018 09:23

bjoerge requested changes Feb 22, 2018

View reviewed changes

rexxars force-pushed the slug-bugs branch from 64c43aa to 7b6230e Compare February 22, 2018 13:14

rexxars requested a review from bjoerge February 22, 2018 13:25

kristofferjs and others added 9 commits February 26, 2018 12:17

[components] Show focus and open on press enter

b60c5ee

[components] Proof of new slug concept

184175b

[form-builder] Remove old validation from slug.

731cfa5

[test-studio] Preview of slug

719977a

[form-builder] Not check slug agains unpublished slugs

8c92b3d

[form-builder] Test of using the validaiton system. Needs access to d…

1a6b076

…ocument

[form-builder] Clean up imports/variables in SlugInput

d349c0c

[form-builder] Add focus handling to slug input

186b3aa

[form-builder] Simplify slug input significantly. Use validation infr…

d4c2c41

…astructure.

rexxars and others added 6 commits February 26, 2018 12:17

[form-builder] Remove unused file

7d5f63f

[form-builder] Make sure _type is set on slug objects

458f332

[form-builder] Make sure slug input is still mounted before setting s…

3cbf4ad

…tate

[form-builder] Make sure slugify always returns a promise

31280af

[form-builder] Fix missing import

350f804

[form-builder] Disable generate slug when no source

cc92dfb

kristofferjs force-pushed the slug-bugs branch from af59a73 to cc92dfb Compare February 26, 2018 11:18

bjoerge approved these changes Feb 28, 2018

View reviewed changes

kristofferjs merged commit 03b7d93 into next Feb 28, 2018

kristofferjs deleted the slug-bugs branch February 28, 2018 09:16

rexxars removed the No docs needed label Feb 28, 2018

rexxars mentioned this pull request Feb 28, 2018

[schema] Deprecate slugifyFn (now called slugify) #618

Merged

evenwestvang mentioned this pull request Mar 13, 2018

Discard changes not working as intended with slug fields #430

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[components] Proof of new slug concept #585

[components] Proof of new slug concept #585

kristofferjs commented Feb 12, 2018

kristofferjs commented Feb 14, 2018

skogsmaskin Feb 15, 2018

kristofferjs Feb 15, 2018

skogsmaskin commented Feb 15, 2018 •

edited

kristofferjs commented Feb 16, 2018

kristofferjs commented Feb 19, 2018

rexxars commented Feb 20, 2018 •

edited

bjoerge left a comment

bjoerge Feb 21, 2018

bjoerge Feb 21, 2018

rexxars Feb 22, 2018

bjoerge Feb 21, 2018

rexxars Feb 22, 2018

bjoerge Feb 21, 2018

bjoerge Feb 21, 2018

bjoerge Feb 21, 2018

bjoerge Feb 21, 2018

bjoerge Feb 21, 2018

rexxars commented Feb 22, 2018

bjoerge commented Feb 22, 2018

rexxars commented Feb 22, 2018

kristofferjs commented Feb 28, 2018

rexxars commented Feb 28, 2018

bjoerge commented Feb 28, 2018

bjoerge commented Feb 28, 2018

rexxars commented Feb 28, 2018

		@@ -9,6 +9,7 @@ export default class ValidationListItem extends React.PureComponent {
		onClick: PropTypes.func,

[components] Proof of new slug concept #585

[components] Proof of new slug concept #585

Conversation

kristofferjs commented Feb 12, 2018

kristofferjs commented Feb 14, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

skogsmaskin commented Feb 15, 2018 • edited

kristofferjs commented Feb 16, 2018

kristofferjs commented Feb 19, 2018

rexxars commented Feb 20, 2018 • edited

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

rexxars commented Feb 22, 2018

bjoerge commented Feb 22, 2018

rexxars commented Feb 22, 2018

kristofferjs commented Feb 28, 2018

rexxars commented Feb 28, 2018

bjoerge commented Feb 28, 2018

bjoerge commented Feb 28, 2018

rexxars commented Feb 28, 2018

skogsmaskin commented Feb 15, 2018 •

edited

rexxars commented Feb 20, 2018 •

edited