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

Enhance ColorPickerWidget #5585

Merged
merged 26 commits into from
Jan 10, 2024
Merged

Enhance ColorPickerWidget #5585

merged 26 commits into from
Jan 10, 2024

Conversation

sneridagh
Copy link
Member

Now it accepts also colors definitions like:

type Color =
  | {
      label: string;
      style: Record<`--${string}`, string> & { name: string };
    }
  | {
      name: string;
      label: string;
    };

const colors: Color[] = [
  { style: { name: 'red', '--background-color': 'red' }, label: 'red' },
  {
    style: { name: 'yellow', '--background-color': 'yellow' },
    label: 'yellow',
  },
  { name: 'green', label: 'green' },
]

So now it supports to save an object containing a list of custom CSS properties definitions.

See documentation for more background.

Copy link

netlify bot commented Jan 2, 2024

Deploy Preview for volto canceled.

Name Link
🔨 Latest commit 86bad73
🔍 Latest deploy log https://app.netlify.com/sites/volto/deploys/659673c1887d8200088869ee

Copy link

netlify bot commented Jan 2, 2024

Deploy Preview for plone-components ready!

Name Link
🔨 Latest commit 86bad73
🔍 Latest deploy log https://app.netlify.com/sites/plone-components/deploys/659673c19a296b00087515d2
😎 Deploy Preview https://deploy-preview-5585--plone-components.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.

@sneridagh sneridagh requested a review from ichim-david January 2, 2024 16:29
@stevepiercy
Copy link
Collaborator

In this comment is the link for the Deploy Preview correct?

https://deploy-preview-5585--plone-components.netlify.app/

I expected to see something like this:

https://6.docs.plone.org/storybook/?path=/story/edit-widgets-colorpicker--default

If it is not correct, then is this a configuration item in GitHub Workflows or Netlify?

Copy link
Collaborator

@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 grammar and MyST syntax fixes, but added some structure and some questions.

docs/source/recipes/how-to-use-color-picker-widget.md Outdated Show resolved Hide resolved
docs/source/recipes/how-to-use-color-picker-widget.md Outdated Show resolved Hide resolved
docs/source/recipes/how-to-use-color-picker-widget.md Outdated Show resolved Hide resolved
docs/source/recipes/how-to-use-color-picker-widget.md Outdated Show resolved Hide resolved
docs/source/recipes/how-to-use-color-picker-widget.md Outdated Show resolved Hide resolved
docs/source/recipes/how-to-use-color-picker-widget.md Outdated Show resolved Hide resolved
docs/source/recipes/how-to-use-color-picker-widget.md Outdated Show resolved Hide resolved
packages/volto/news/5585.feature Outdated Show resolved Hide resolved
docs/source/recipes/how-to-use-color-picker-widget.md Outdated Show resolved Hide resolved
docs/source/recipes/how-to-use-color-picker-widget.md Outdated Show resolved Hide resolved
@sneridagh
Copy link
Member Author

In this comment is the link for the Deploy Preview correct?

https://deploy-preview-5585--plone-components.netlify.app/

I expected to see something like this:

https://6.docs.plone.org/storybook/?path=/story/edit-widgets-colorpicker--default

If it is not correct, then is this a configuration item in GitHub Workflows or Netlify?

No, it's not correct. Good catch. I configured the @plone/components package in the monorepo to have also a build. Github should be confusing both. I guess it has to do with the monorepo setup in Netlify. I will take a look.

@sneridagh
Copy link
Member Author

@stevepiercy the emphasize-lines does not work :( Something we can do?

@stevepiercy
Copy link
Collaborator

@sneridagh I will poke around inside Netlify and GitHub actions and Makefile. There must be something in there that triggers it.

@stevepiercy
Copy link
Collaborator

@sneridagh needs a change log entry.

@stevepiercy
Copy link
Collaborator

@sneridagh also unit tests fail in CI.

Co-authored-by: Steve Piercy <web@stevepiercy.com>
@stevepiercy
Copy link
Collaborator

@sneridagh I found it. There is another site in Netlify, https://app.netlify.com/sites/plone-components/overview, that gets built on every pull request in Volto. I think we want it to build only when something in its package builds, not within the entire volto repo, correct?

@sneridagh
Copy link
Member Author

@stevepiercy I will take a look at the unit tests I still want to polish the code a bit.

@sneridagh
Copy link
Member Author

@stevepiercy is there a way to filter when to build it?

@stevepiercy
Copy link
Collaborator

@stevepiercy is there a way to filter when to build it?

Yup, similar to what we do for Volto docs, we can do a diff. Let's do this in a separate PR, because I don't want this one to become a victim of feature creep. I also want to add the Storybook build for docs to be reachable at volto-docs-preview-url/storybook so we can begin to use arbitrary relative links to it. I'll do both in a separate PR.

docs/source/recipes/color-picker-widget.md Show resolved Hide resolved
docs/source/recipes/color-picker-widget.md Outdated Show resolved Hide resolved
docs/source/recipes/color-picker-widget.md Outdated Show resolved Hide resolved
docs/source/recipes/color-picker-widget.md Outdated Show resolved Hide resolved
return (
<Button
key={id + color.name}
className={color.name}
key={id + colorName}
Copy link
Member

Choose a reason for hiding this comment

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

I think we could just use the index in colors here and avoid requiring name. It seems unlikely that they will be reordered frequently.

Copy link
Member Author

Choose a reason for hiding this comment

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

@davisagli so you'd change the style use case branch? as in:

type Color =
  | {
      name: undefined;
      label: string;
      style: Record<`--${string}`, string>;
    }
  | {
      name: string;
      label: string;
      style: undefined;
    };

Copy link
Member

Choose a reason for hiding this comment

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

@sneridagh yeah, that's the suggestion, but I don't feel strongly about it

Copy link
Member Author

@sneridagh sneridagh Jan 4, 2024

Choose a reason for hiding this comment

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

@davisagli I don't know, I feel the way it is now is more consistent, when we deal with choices, we always have a "display name" (label) and a "token" (value). The token is invariable, but the display name could be i18n'd. If at some point you need to identify the color entry, you will have to equal the style property (which you could still do...) Answering myself while writing :)

Anyways, I don't have a strong opinion either at this point.

/cc @tiberiuichim @ichim-david @giuliaghisini @pnicolli any opinions about this?

sneridagh and others added 2 commits January 3, 2024 20:10
Co-authored-by: David Glick <david@glicksoftware.com>
Copy link
Collaborator

@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.

Looking good: https://deploy-preview-5585--volto.netlify.app/recipes/color-picker-widget.html

Just a minor suggestion to the news item, and it's GTG.

docs/source/recipes/how-to-use-color-picker-widget.md Outdated Show resolved Hide resolved
packages/scripts/news/5585.feature Outdated Show resolved Hide resolved
Co-authored-by: Steve Piercy <web@stevepiercy.com>
@sneridagh sneridagh requested a review from tiberiuichim January 4, 2024 09:07
@sneridagh sneridagh merged commit 90fbdff into main Jan 10, 2024
64 checks passed
@sneridagh sneridagh deleted the enhanceColorPickerWidget branch January 10, 2024 10:55
sneridagh added a commit that referenced this pull request Jan 10, 2024
Co-authored-by: Steve Piercy <web@stevepiercy.com>
Co-authored-by: ichim-david <ichim.david@gmail.com>
Co-authored-by: David Glick <david@glicksoftware.com>
sneridagh added a commit that referenced this pull request Jan 11, 2024
Co-authored-by: Steve Piercy <web@stevepiercy.com>
Co-authored-by: ichim-david <ichim.david@gmail.com>
Co-authored-by: David Glick <david@glicksoftware.com>
sneridagh added a commit that referenced this pull request Jan 17, 2024
* main: (1505 commits)
  Links references view cypress tests (#5111)
  Unify start command, trigger `build:deps` command too (#5633)
  Release generate-volto 9.0.0-alpha.3
  Release @plone/types 1.0.0-alpha.2
  Improve Blocks typings. Refactor `volto-coresandbox` to TS. (#5624)
  i18n polishing (#5542)
  Release @plone/scripts 3.2.1
  Fix @plone/scripts for 17 and below (#5613)
  Release @plone/scripts 3.2.0
  Enhance `ColorPickerWidget` (#5585)
  edit recurrence button is aligned properly (#5590)
  Revert "Attempt to build the volto/components Storybook only when something c…" (#5601)
  Revert "Added Storybook to Netlify build to allow relative links from documen…" (#5598)
  Added Storybook to Netlify build to allow relative links from documen… (#5596)
  Attempt to build the volto/components Storybook only when something c… (#5595)
  Pin mrs.developer to an updated version, never to star. (#5593)
  Allow to bail off the nested name build in the custom CSS properties style name generator (#5586)
  Clarify how CSS properties work (#5592)
  Release 18.0.0-alpha.6
  Release @plone/client 1.0.0-alpha.12
  ...
sneridagh added a commit that referenced this pull request Jan 21, 2024
* main: (39 commits)
  Attempt to build the components storybook only when its source files … (#5600)
  Added Storybook to Netlify build to allow relative links from documentation (#5599)
  Fix multilingual redirector (#5628) (#5665)
  Update ref from install-from-packages to create-project (#5654)
  Pin Vale to 2.30.0 to allow build of documentation until we can upgrade to v3.x (#5656)
  Temporarily pin `sphinxcontrib-*help` dependencies so docs can build (#5655)
  Merge the StyleWrapper styles with the draggable props from b-D&D (#5652)
  Fix order of preference in addons-registry for the theme (#5649)
  Unify variables in Makefile (#5637)
  Links references view cypress tests (#5111)
  Unify start command, trigger `build:deps` command too (#5633)
  Release generate-volto 9.0.0-alpha.3
  Release @plone/types 1.0.0-alpha.2
  Improve Blocks typings. Refactor `volto-coresandbox` to TS. (#5624)
  i18n polishing (#5542)
  Release @plone/scripts 3.2.1
  Fix @plone/scripts for 17 and below (#5613)
  Release @plone/scripts 3.2.0
  Enhance `ColorPickerWidget` (#5585)
  edit recurrence button is aligned properly (#5590)
  ...
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.

5 participants