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

Less flickering #94

Merged
merged 3 commits into from
Oct 2, 2024
Merged

Conversation

kitzberger
Copy link
Contributor

@kitzberger kitzberger commented Jul 30, 2024

To be discussed...

Resolves: #93

@Patta
Copy link
Contributor

Patta commented Jul 31, 2024

@kitzberger nice, I will test this as soon as possible.

First question:
Where does the integrator have to insert the viewhelper code mentioned in the readme?
In the powermail layout or page layout/template? Then the code would always be inserted, regardless of whether powermail_cond is used on the page or not. I think it would be better if the condition code is added directly with powermail_cond, perhaps with its own powermail_cond fluid layout/template, which does not have to be set separately by the integrator.

@kitzberger
Copy link
Contributor Author

kitzberger commented Jul 31, 2024

@Patta, since EXT:powermail_cond doesn't come with any templates/partials overrides so far, I figured let's keep it that way. That's why (at least for this early stage of the patch) the integrator has to insert that snippet manually.

The code can be inserted into powermail's form template!

@kitzberger kitzberger force-pushed the less-flickering branch 2 times, most recently from 41539de to 2726d73 Compare August 22, 2024 14:17
@sbusemann
Copy link
Collaborator

Copy link
Member

@pixeldesu pixeldesu left a comment

Choose a reason for hiding this comment

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

Thank you for your contribution!

From initial testing, this seems like a great addition, but I do have some remarks for the code that should get adjusted before we merge this into the project!

  • Please remove debug outputs, this mainly concerns the console.log statements in the JavaScript code. If you need to describe the conditional branches, feel free to use comments instead!
  • This is mainly a structuring/"meaning" thing, but I don't think that ConditionUtility is a utility. Technically, you moved the domain logic of the controller, including fetching of forms and conditions somewhere else, so it's probably more appropriate to name this ConditionService instead (and move it into a Service folder too)

Once these adjustments have been made, things should be ready to merge this!

@kitzberger kitzberger force-pushed the less-flickering branch 2 times, most recently from 92ff381 to ac2d2e5 Compare September 26, 2024 08:45
@kitzberger kitzberger changed the base branch from master to develop September 26, 2024 08:45
@kitzberger
Copy link
Contributor Author

@pixeldesu, thanks for your feedback! I removed all console.log's that I've added, changed the utility's class name and rebase everything to the latest develop branch.

@pixeldesu pixeldesu self-requested a review October 2, 2024 08:59
Copy link
Member

@pixeldesu pixeldesu left a comment

Choose a reason for hiding this comment

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

Thanks! Tested it and works fine! 👍

@pixeldesu pixeldesu merged commit 0b472cc into in2code-de:develop Oct 2, 2024
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Flickering of initially hidden fields
4 participants