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

IBX-2624: Disabled validation for discard changes #602

Merged
merged 3 commits into from
Oct 17, 2022

Conversation

lucasOsti
Copy link
Contributor

@lucasOsti lucasOsti commented Oct 12, 2022

Question Answer
Tickets https://issues.ibexa.co/browse/IBX-2624
Releated PR https://github.com/ibexa/product-catalog/pull/824
Bug fix? yes
New feature? no
BC breaks? no
Tests pass? yes
Doc needed? no
License GPL-2.0

Checklist:

  • Coding standards ($ composer fix-cs)
  • Ready for Code Review

@lucasOsti lucasOsti force-pushed the IBX-2624-discard-changes-without-validation branch from 7668193 to 2e71271 Compare October 12, 2022 12:34
@lucasOsti lucasOsti changed the base branch from main to 4.2 October 12, 2022 12:40
@lucasOsti lucasOsti changed the title Disabled validation for discard changes IBX-2624: Disabled validation for discard changes Oct 12, 2022
@lucasOsti lucasOsti added Bug Something isn't working Ready for review labels Oct 13, 2022

validateForm();
if (!submitter.classList.contains('ibexa-content-type-edit__remove-draft')) {
Copy link
Contributor

Choose a reason for hiding this comment

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

submitter can be null if the event was not triggered by an input/button.
ref. https://developer.mozilla.org/en-US/docs/Web/API/SubmitEvent/submitter

Copy link
Contributor Author

@lucasOsti lucasOsti Oct 13, 2022

Choose a reason for hiding this comment

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

This should not be a problem here because in this case click to submit button is done via the button from context menu but I will add optional chaining it should fix possible problems in the future if this even would be emitted from another element

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, but someone may have a custom script here or we may add something in the future. Adding optional chaining creates a more robust solution.

Copy link
Contributor

Choose a reason for hiding this comment

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

what do you think about using the attribute formnovalidate for this if (we may have more buttons which should not validate form)?
https://developer.mozilla.org/en-US/docs/Web/HTML/Element/button#attr-formnovalidate


validateForm();
if (!submitter?.getAttribute('formnovalidate')) {
Copy link
Contributor

@tischsoic tischsoic Oct 14, 2022

Choose a reason for hiding this comment

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

To be specific, getAttribute won't work for formnovalidate="" which is a valid case.

Suggested change
if (!submitter?.getAttribute('formnovalidate')) {
if (!submitter?.hasAttribute('formnovalidate')) {

@sonarcloud
Copy link

sonarcloud bot commented Oct 14, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@bogusez
Copy link
Contributor

bogusez commented Oct 17, 2022

Regression tests passed: ibexa/commerce#142

@dew326 dew326 merged commit a541031 into 4.2 Oct 17, 2022
@dew326 dew326 deleted the IBX-2624-discard-changes-without-validation branch October 17, 2022 06:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants