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

fix(DPLAN-2231): fix 'data-dp-validate-if' within a fieldset #850

Merged
merged 10 commits into from
May 17, 2024

Conversation

sakutademos
Copy link
Contributor

@sakutademos sakutademos commented May 14, 2024

Ticket: DPLAN-2231

Description: This PR fixes an issue with validation when a 'data-dp-validate-if' attribute is provided in an input within a fieldset instead of directly within a form, because the query selector (line 20) should search the entire form rather than just the fieldset; otherwise, it returns null.

  • check if a fieldset is provided instead of a form when using input.closest('[data-dp-validate]')
  • cognitive complexity has been reduced for the 'assignHandlersForSingleInput' method to address a SonarCloud issue

… expected when a fieldset is provided instead of a form, so check additionally if a fieldset provided, reduce cognitive complexity for the assignHandlersForSingleInput method
@@ -21,7 +21,8 @@ function assignHandlersForSingleInput (input) {
allConditions.forEach(condition => {
const comparisonType = condition.indexOf('!==') > -1 ? 'isNotEqual' : 'isEqual'
const matchers = condition.split(comparisonType === 'isNotEqual' ? '!==' : '===')
const form = input.closest('[data-dp-validate]')
const validationContainer = input.closest('[data-dp-validate]')
const form = validationContainer.tagName === 'FIELDSET' && validationContainer.form ? validationContainer.form : validationContainer
Copy link
Contributor

Choose a reason for hiding this comment

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

I feel this logic needs a little commenting.

Also, i think we should move the dp-validate documentation from demosplan-core into demosplan-ui and check if it is outdated (certainly not in this PR).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

add a comment here ae879da

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We currently have a Storybook in demosplan-ui. What do you mean by move it into demosplan-ui? create a docu for demosplan-ui?

Copy link
Contributor

@spiess-demos spiess-demos May 17, 2024

Choose a reason for hiding this comment

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

Correct. At the moment we document only components and tokens via Storybook. Neither "directives", "lib" (which is highly unsorted and should be sorted upfront), "mixins" or "utils" are documented at all. At a second glance i think we should at first decide how to structure util/lib code within demosplan-ui, and only after that document things we decided to be "multi purpose enough" to stay in demosplan-ui.

Copy link
Contributor

@hwiem hwiem left a comment

Choose a reason for hiding this comment

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

I agree that an explanatory comment would be helpful (what @spiess-demos said).
Code lgtm, just a comment on the changelog entry

CHANGELOG.md Outdated Show resolved Hide resolved
Copy link
Contributor

@spiess-demos spiess-demos left a comment

Choose a reason for hiding this comment

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

lgtm (code review)

@sakutademos sakutademos merged commit dbdda59 into main May 17, 2024
1 check passed
@sakutademos sakutademos deleted the fix-DPLAN-2231-validate-if-issue-with-the-fieldset branch May 17, 2024 11:49
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.

3 participants