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

3716 - improve validation of id widget #4686

Merged
merged 47 commits into from
Jan 26, 2024

Conversation

tedw87
Copy link
Contributor

@tedw87 tedw87 commented Apr 12, 2023

I modified the regex which validates the value in IdWidget to match all the requirements mentioned in the issue #3716

@netlify
Copy link

netlify bot commented Apr 12, 2023

Deploy Preview for volto ready!

Name Link
🔨 Latest commit 51c235f
🔍 Latest deploy log https://app.netlify.com/sites/volto/deploys/65b3723e84e24c00082b1fab
😎 Deploy Preview https://deploy-preview-4686--volto.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.

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.

A few questions. I don't know the answers, but the content seems a bit off to me.

@cypress
Copy link

cypress bot commented Apr 12, 2023

Passing run #5051 ↗︎

0 493 20 0 Flakiness 0

Details:

Merge branch 'master' into 3716-improve-validation-of-IdWidget
Project: Volto Commit: 5b5965f887
Status: Passed Duration: 14:37 💡
Started: Apr 27, 2023 5:18 AM Ended: Apr 27, 2023 5:33 AM

This comment has been generated by cypress-bot as a result of this project's GitHub integration settings.

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.

LGTM, of course, I can only vouch for the English version. Thank you!

@erral

This comment was marked as outdated.

@sneridagh
Copy link
Member

@tedw87 status?

@sneridagh sneridagh requested a review from davisagli September 12, 2023 08:48
@sneridagh
Copy link
Member

@davisagli could you please take a second look?

@sneridagh
Copy link
Member

@tedw87 sorry to have this abandoned. What's it's status? Can we resurect it? Thanks!

@sneridagh sneridagh added this to the 18.x.x milestone Nov 4, 2023
@tedw87
Copy link
Contributor Author

tedw87 commented Nov 6, 2023

@tedw87 sorry to have this abandoned. What's it's status? Can we resurect it? Thanks!

@sneridagh Hello, the updates will be as follows: 1. No uppercase letters are to be used; 2. 'contributors' should not be used as a shortname. I will update the validation criteria accordingly. Thank you.

@tedw87
Copy link
Contributor Author

tedw87 commented Jan 11, 2024

@stevepiercy I've updated the Storybook by including Errored. If anyone has suggestions or feedback, please feel free to share them.
Screenshot from 2024-01-11 18-43-20

Copy link
Member

@sneridagh sneridagh left a comment

Choose a reason for hiding this comment

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

LGTM but not a regex expert here! @stevepiercy @davisagli a final look?

@stevepiercy
Copy link
Collaborator

@sneridagh the Storybook preview does not work. I have no idea why:

https://65b0360fb762f40008f1c5f1--volto.netlify.app/storybook

I thought I had fixed that in #5599 (comment), but it appears to have been broken in this PR.

If it works locally, then I'm fine with Netlify not working and find where it went sideways later.


As far as the regex, it appears that it does not reject \n or \r. See regex tester at https://regex101.com/r/E5gJye/1.

@stevepiercy
Copy link
Collaborator

It looks like the latest regex works: https://regex101.com/r/E5gJye/3.

That leaves the Storybook build failure to resolve. I forced it to rebuild with the latest commit, but it still does not load: https://deploy-preview-4686--volto.netlify.app/storybook

@tedw87
Copy link
Contributor Author

tedw87 commented Jan 25, 2024

It looks like the latest regex works: https://regex101.com/r/E5gJye/3.

That leaves the Storybook build failure to resolve. I forced it to rebuild with the latest commit, but it still does not load: https://deploy-preview-4686--volto.netlify.app/storybook

I ran Storybook locally on this branch and it works fine. I tested it in the following way: after make storybook-build, I run serve at this path volto/docs/_build/html/storybook.

@stevepiercy
Copy link
Collaborator

Thanks for checking @tedw87! @sneridagh let's go ahead and merge this PR, and if the Storybook does not build and deploy after I update git submodule tips on the Plone 6 documentation, we can open a new issue to deal with it there.

@sneridagh sneridagh merged commit bb1753d into main Jan 26, 2024
63 checks passed
@sneridagh sneridagh deleted the 3716-improve-validation-of-IdWidget branch January 26, 2024 09:36
@stevepiercy
Copy link
Collaborator

The Storybook build is fine in production at https://6.docs.plone.org/storybook/, so something with the Volto docs preview in Netlify might need some tweaking.

sneridagh added a commit that referenced this pull request Feb 1, 2024
* main:
  Block search engines from indexing content on Netlify preview builds (#5725)
  fixed a11y Logo Issue and added Translations (#5722)
  Allow editor to edit metadata name during bulk or single upload (#5560)
  5439 clarify defaultAddonName option and mention to the generator readme the creation of the theme add-on (#5709)
  Remove turbo from monorepo commands until it's really necessary (#5715)
  bugfix: wrong conditional proprieties on ObjectBrowser (#4190)
  Added aria-live='polite' (#5639)
  Release 18.0.0-alpha.9
  Release @plone/scripts 3.3.2
  fix: handle addons that have not been migrated to the new structure o… (#5704)
  Update Semantic UI React to version 2.1.5 (#5632)
  Removed unmaintained and unused razzle-plugin-bundle-analyze (#5671)
  3716 - improve validation of id widget (#4686)
  Fixed redirect of https://tanstack.com/query/v4/docs/react/guides/ssr… (#5700)
  Release 18.0.0-alpha.8
  Release @plone/volto 18.0.0-alpha.7 (not pushed to main)
  Release @plone/slate 18.0.0-alpha.6
  Release @plone/registry 1.2.1 (not pushed to repo)
sneridagh pushed a commit that referenced this pull request Feb 2, 2024
Co-authored-by: Claudia Ifrim <ifrim.claudia@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

6 participants