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.

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

Add plone.autoform documentation for classic UI #1571

Merged
merged 16 commits into from
Apr 17, 2024
Merged

Conversation

petschki
Copy link
Member

@petschki petschki commented Nov 6, 2023

Put plone.autoform documentation to the designated places.

Copy link

netlify bot commented Nov 6, 2023

Deploy Preview for 6-docs-plone-org ready!

Name Link
🔨 Latest commit b0e2f63
🔍 Latest deploy log https://app.netlify.com/sites/6-docs-plone-org/deploys/661f812344c9a90008c75348
😎 Deploy Preview https://deploy-preview-1571--6-docs-plone-org.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

docs/classic-ui/forms.md Outdated Show resolved Hide resolved
docs/classic-ui/forms.md Outdated Show resolved Hide resolved
docs/classic-ui/forms.md Outdated Show resolved Hide resolved
docs/classic-ui/forms.md Outdated Show resolved Hide resolved
docs/classic-ui/forms.md Outdated Show resolved Hide resolved
docs/classic-ui/forms.md Outdated Show resolved Hide resolved
Copy link
Contributor

@stevepiercy stevepiercy left a comment

Choose a reason for hiding this comment

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

Mostly a bunch of style guide suggestions.

  • Please use one line per sentence. Documentation is not code.
  • Dedent code examples to avoid horizontal scrolling.
  • Some missing inline literals.

Also I agree with and defer to @MrTango suggestions for where topics belong. I would replace the content with reference links to the authoritative content. That might require adding references and targets in MyST syntax.

For example:

```{seealso}
{ref}`Referenced section <target>`
```

and in the target section:

```
(target)=
```

docs/classic-ui/forms.md Outdated Show resolved Hide resolved
docs/classic-ui/forms.md Outdated Show resolved Hide resolved
docs/classic-ui/forms.md Outdated Show resolved Hide resolved
docs/classic-ui/forms.md Outdated Show resolved Hide resolved
docs/classic-ui/forms.md Outdated Show resolved Hide resolved
docs/classic-ui/forms.md Show resolved Hide resolved
docs/classic-ui/forms.md Show resolved Hide resolved
docs/classic-ui/forms.md Outdated Show resolved Hide resolved
docs/classic-ui/forms.md Outdated Show resolved Hide resolved
docs/classic-ui/forms.md Outdated Show resolved Hide resolved
@petschki petschki force-pushed the classic-ui-autoforms branch 2 times, most recently from c9ebffa to 6a56c28 Compare November 9, 2023 15:03
@petschki
Copy link
Member Author

petschki commented Nov 9, 2023

@petschki petschki force-pushed the classic-ui-autoforms branch 2 times, most recently from 8661e86 to 8fd6657 Compare November 9, 2023 15:15
Copy link
Contributor

@stevepiercy stevepiercy left a comment

Choose a reason for hiding this comment

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

Do we need the u in u"value" in Plone 6? I would prefer to remove all these instances, if not needed.

Otherwise I pushed a few commits to tidy this up and push it over the finish line. It requires a core contributor or maintainer to review its technical accuracy.

docs/backend/schemas.md Outdated Show resolved Hide resolved
jensens
jensens previously approved these changes Mar 20, 2024
Copy link
Sponsor Member

@jensens jensens left a comment

Choose a reason for hiding this comment

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

Very important! LGTM, just one question.

docs/backend/schemas.md Outdated Show resolved Hide resolved
Copy link
Contributor

@stevepiercy stevepiercy left a comment

Choose a reason for hiding this comment

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

LGTM. One more pass from @jensens, or anyone else, and we can merge. Thank you!

@MrTango
Copy link
Contributor

MrTango commented Apr 11, 2024

I've also removed this part:

https://github.com/plone/documentation/pull/1571/files#diff-2a092039aef3cb61f8a3fdc888afaaca68f5dd8a64b6b612cbf3c3d2af1b01d4L591-L603

plone.directives.form is outdated and the default value hint is already covered here https://github.com/plone/documentation/pull/1571/files#diff-2a092039aef3cb61f8a3fdc888afaaca68f5dd8a64b6b612cbf3c3d2af1b01d4L591-L603

i can't find the default value info, you want to remove, elsewhere.
Where do you see them?

Copy link
Contributor

@MrTango MrTango left a comment

Choose a reason for hiding this comment

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

Except the default value part you want to remove, LGTM. feel free to merge

@stevepiercy
Copy link
Contributor

Except the default value part you want to remove, LGTM. feel free to merge

@MrTango do you mean that you want @petschki to restore the default value part that he removed? I'm confused because you also said "feel free to merge". Please clarify.

@MrTango
Copy link
Contributor

MrTango commented Apr 11, 2024

Except the default value part you want to remove, LGTM. feel free to merge

@MrTango do you mean that you want @petschki to restore the default value part that he removed? I'm confused because you also said "feel free to merge". Please clarify.

yes, he said that what he want's to remove is described elsewhere, but i could not find it.
So either enlighten me and remove it or keep it.
But let him do it, no need to act for you here.

@petschki
Copy link
Member Author

Sorry for the confusion. I've only de-duplicated the "default value hint" in schemas.md and remove the old link to plone.directives.form ... the hint is already in the warning box here https://6.docs.plone.org/backend/schemas.html?highlight=default%20value#field-constructor-parameters ... so all good from my side to merge.

@stevepiercy
Copy link
Contributor

@petschki thanks for the clarification. I'll merge when it is green. Thank you!

@stevepiercy
Copy link
Contributor

There was a broken link to the TinyMCE 4 to 5 upgrade guide. I also created a new issue to upgrade to the latest version TinyMCE 7, as v5 is EOL. plone/Products.CMFPlone#3936

@stevepiercy stevepiercy merged commit a8c9d60 into 6.0 Apr 17, 2024
6 checks passed
@stevepiercy stevepiercy deleted the classic-ui-autoforms branch April 17, 2024 08:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants