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

Update widget templates with newest z3c.form extendable attributes. #181

Merged
merged 33 commits into from Jul 28, 2023

Conversation

petschki
Copy link
Member

@petschki petschki commented Jul 19, 2023

This PR proposes:

  • cleaning up widget templates with new chameleon attribute interpolation wich is possible since z3c.form>=5.1.
  • getting rid of lxml pattern widget creation in favor of z3c.form extendable attributes.
  • cleaning up browser add/edit view overrides as this will be no longer needed for pattern widget rendering.

Point 2 and 3 came into my mind while cleaning up some widget templates. I'll come up with some commits which will outline my idea of how to get rid of the lxml created widget markup.

This is a major but internal overhaul of the z3c.form widget override story and should not break any core packages. Though if someone has made z3c.jbot overrides of plone.app.z3cform widget templates s/he must update them accordingly (or remove them completely in favor of the new implementation).

@mister-roboto
Copy link

@petschki thanks for creating this Pull Request and helping to improve Plone!

TL;DR: Finish pushing changes, pass all other checks, then paste a comment:

@jenkins-plone-org please run jobs

To ensure that these changes do not break other parts of Plone, the Plone test suite matrix needs to pass, but it takes 30-60 min. Other CI checks are usually much faster and the Plone Jenkins resources are limited, so when done pushing changes and all other checks pass either start all Jenkins PR jobs yourself, or simply add the comment above in this PR to start all the jobs automatically.

Happy hacking!

plone/app/z3cform/widgets/text.py Outdated Show resolved Hide resolved
plone/app/z3cform/templates/text_input.pt Show resolved Hide resolved
@gforcada
Copy link
Sponsor Contributor

The CI failures for test and coverage is about the new version pin.

On our constraints.txt file we have z3c-form==4.3 while we define on setup.py z3c-form>=5.1.

Can we get a PR that updates z3c.form on buildout.coredev first? 🤞🏾

Once that passes, we need to update the constraints file (so far @mauritsvanrees work, sorry to put more burden on you 😓 ) and finally we can get this PR to be tested 🎉

It's a bit of a 🐜 🐜 🐜 work, but this way we ensure all pipelines from all packages keep working 😅

Whenever we introduce mxdev, we will be able to, at least locally, check the package before we have a newer version pin blessed generally, which can be a big improvement IMHO... some more work on each PR probably, but if we want to get CI to work always, we have to put some discipline and effort to it 😅

/cc @ericof @jensens @fredvd @mauritsvanrees how does that sound?

Copy link
Sponsor Contributor

@gforcada gforcada left a comment

Choose a reason for hiding this comment

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

Changes LGTM, we need to fix the pipeline, as I mentioned in another comment, and resolve the changes proposed by @thet 👍🏾

Looks great! 🌟

@petschki
Copy link
Member Author

petschki commented Jul 20, 2023

I would say, these changes target Plone 6.1+. I can create a 4.3.x branch here for the buildout.coredev=6.0 branch and move on with version 4.4.x on master branch ...

@petschki
Copy link
Member Author

I saw that I'm able to override the constraints with .meta.toml configuration ... nice. But there is no https://dist.plone.org/release/6.1-dev/constraints.txt 😅

@mauritsvanrees
Copy link
Sponsor Member

The changes seem better for 6.1 indeed.

I have created a very preliminary version of https://dist.plone.org/release/6.1-dev/
I copied the requirements.txt and constraints.txt from 6.0-dev, and changed the pins for z3c.form and plone.app.multilingual.
There is a good chance that this will only work if you have a checkout of Products.CMFPlone and Plone. I did not try it out at all. But at least there is something to play with now.

@petschki
Copy link
Member Author

Thank you! I'll give it a try now.

@petschki
Copy link
Member Author

Well @gforcada the new constraints.txt from @mauritsvanrees did the trick ... implemented in .meta.toml... no need for extra checkouts 🎉

@petschki petschki force-pushed the petschki-z3cform-update branch 3 times, most recently from c85bb1e to abe1213 Compare July 20, 2023 16:45
@petschki
Copy link
Member Author

petschki commented Jul 20, 2023

There we go ... this is the new base for "pattern" widgets: 2c7558b ... the pattern_options are set inside attributes and the pattern class in update() to ensure correct css class stacking.

The QueryStringWidget is a simple example of the new implementation: abe1213

/cc @gforcada @thet

@mauritsvanrees
Copy link
Sponsor Member

Hurray! That looks much simpler!

@petschki petschki force-pushed the petschki-z3cform-update branch 5 times, most recently from e0543c7 to fbd528e Compare July 21, 2023 09:45
@petschki
Copy link
Member Author

The new button implementation also fixes #142 as we do not need the add/edit form overrides for ActionButtons anymore.

@petschki
Copy link
Member Author

@gforcada the meta/dependencies checker says that plone.dexterity is missing in test requiries but it is defined here: https://github.com/plone/plone.app.z3cform/pull/181/files#diff-60f61ab7a8d1910d86d9fda2261620314edcae5894d5aaa236b821c7256badd7R19

I do not really know how to fix that?

@petschki petschki force-pushed the petschki-z3cform-update branch 4 times, most recently from 8d8f10a to affcebe Compare July 27, 2023 12:39
@mauritsvanrees
Copy link
Sponsor Member

@gforcada the meta/dependencies checker says that plone.dexterity is missing in test requiries but it is defined here: https://github.com/plone/plone.app.z3cform/pull/181/files#diff-60f61ab7a8d1910d86d9fda2261620314edcae5894d5aaa236b821c7256badd7R19

I do not really know how to fix that?

The problem is that plone.dexterity is both in install_requires and in test. Since it is now only used in tests, it should be removed from install_requires.

I guess the message could be a bit clearer.

@petschki
Copy link
Member Author

The problem is that plone.dexterity is both in install_requires and in test. Since it is now only used in tests, it should be removed from install_requires.

I guess the message could be a bit clearer.

Thanks! Now I have to take a look why the build is broken 😅

@petschki petschki force-pushed the petschki-z3cform-update branch 2 times, most recently from 1bb3f7d to aea9d41 Compare July 28, 2023 06:52
plone/app/z3cform/templates/checkbox_input.pt Outdated Show resolved Hide resolved
plone/app/z3cform/templates/checkbox_input.pt Outdated Show resolved Hide resolved
plone/app/z3cform/templates/file_input.pt Outdated Show resolved Hide resolved
plone/app/z3cform/templates/file_input.pt Show resolved Hide resolved
plone/app/z3cform/templates/orderedselect_input.pt Outdated Show resolved Hide resolved
plone/app/z3cform/templates/radio_input_single.pt Outdated Show resolved Hide resolved
plone/app/z3cform/templates/checkbox_input.pt Outdated Show resolved Hide resolved
plone/app/z3cform/templates/select_input.pt Show resolved Hide resolved
@petschki petschki mentioned this pull request Jul 28, 2023
@petschki
Copy link
Member Author

Build is green now. @thet wanna take a look?

@thet thet merged commit 9f67fdc into master Jul 28, 2023
10 checks passed
@thet thet deleted the petschki-z3cform-update branch July 28, 2023 13:20
@MrTango
Copy link
Contributor

MrTango commented Nov 3, 2023

Is there anything we should update in docs, for this changes?
Forms/Widgets?

@petschki
Copy link
Member Author

petschki commented Nov 3, 2023

Not for this change. This is almost all internal stuff. But the widget import location should be changed in the docs. Since version 4.0.x we've implemented all z3c.form browser widgets with Bootstrap 5 flavor here. I'll update it in the doc PR.

@petschki
Copy link
Member Author

petschki commented Nov 3, 2023

Update: the imports are fine for now. We need to think about this when we update the widget docs for Plone 6.1 ...

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

6 participants