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: Do not add advanced prop on submit #436

Conversation

timagixe
Copy link
Contributor

Description

  • maps option colors with default ones
  • does not set advanced prop on submit

Fixes #432

Type of change

  • Bug fix (added a non-breaking change which fixes an issue)
  • New feature (added a non-breaking change which adds functionality)
  • Updated documentation (updated the readme, templates, or other repo files)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

How Has This Been Tested?

  • Tested locally with a valid username
  • Tested locally with an invalid username
  • Ran tests with composer test
  • Added or updated test cases to test new features

Checklist:

  • I have checked to make sure no other pull requests are open for this issue
  • The code is properly formatted and is consistent with the existing code style
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings

Screenshots

N/A

@DenverCoder1
Copy link
Owner

DenverCoder1 commented Jan 29, 2023

In my opinion, it looks better without the default option and the red color helps it stand out so it's easier to tell which attribute was added.

I think a simpler solution to the issue would be to just add the type="button" attribute to all the buttons to make them not treated as submit on Enter.

Feel free to let me know what you think.

@timagixe
Copy link
Contributor Author

timagixe commented Jan 30, 2023

I hope I got your right, I have tried the following and it didn't work:

- <input class="btn" type="submit" value="Open Permalink">
+ <input class="btn" type="button" value="Open Permalink">

With the change above, pressing Enter when working with form still adds advanced properties.

One of the below changes works fine:

  1. add event listener to form to prevent default behaviour when Enter is pressed
diff --git a/src/demo/js/script.js b/src/demo/js/script.js

 document.addEventListener("click", () => preview.update(), false);
+document.querySelector("form").addEventListener("keydown", (event) => {
+  if (event.key === 'Enter') event.preventDefault();
+});
  1. add onkeydown handler to form element
diff --git a/src/demo/index.php b/src/demo/index.php

             <h2>Properties</h2>
-            <form class="parameters">
+            <form class="parameters" onkeydown="return event.key != 'Enter';">
                 <label for="user">Username<span title="required">*</span></label>

@DenverCoder1 please let me know what you think of it. Which one is more preferable or maybe you have a different vision of it. Thanks.

@DenverCoder1
Copy link
Owner

The reason it adds a property is because Enter triggers a form submit so it finds the first button that does not have the type="button" attribute or an input that has type="submit". In this case, the Add Property button is the first button in the form so it is used as the "submit" action since it does not have type="button".

Since it is not meant to submit the form, the add property button and remove property buttons should have type="button". The copy button doesn't really need it since it's not inside a form, but it can't hurt to have the attribute just to signify that it is not a submit action. The Open Permalink button could be a submit action, but I don't know if it's really necessary to have it submit, so I think it's fine either way.

Prevent add property buttons from submitting:

--- a/src/demo/index.php
+++ b/src/demo/index.php

-                        <button class="plus btn" onclick="return preview.addProperty();">+</button>
+                        <button type="button" class="plus btn" onclick="return preview.addProperty();">+</button>

Prevent remove property buttons from submitting:

--- a/src/demo/js/script.js
+++ b/src/demo/js/script.js

       minus.innerText = "−";
+      minus.setAttribute("type", "button");
       minus.setAttribute("data-property", propertyName);

Not required -- Prevent "open permalink" from submitting and signify semantically that copy is not a submit:

--- a/src/demo/index.php
+++ b/src/demo/index.php

-                <input class="btn" type="submit" value="Open Permalink">
+                <input class="btn" type="button" value="Open Permalink">

-                <button class="copy-button btn tooltip" onclick="clipboard.copy(this);" onmouseout="tooltip.reset(this);" disabled>
+                <button type="button" class="copy-button btn tooltip" onclick="clipboard.copy(this);" onmouseout="tooltip.reset(this);" disabled>

Let me know if you think this makes sense. I've tested it and it works for me.

@timagixe timagixe force-pushed the issue-432-do-not-add-advanced-prop-on-submit branch from 33d6e4d to 50773aa Compare January 30, 2023 17:44
timagixe and others added 3 commits January 30, 2023 19:49
Co-authored-by: DenverCoder1 <jonah@freshidea.com>
Co-authored-by: DenverCoder1 <jonah@freshidea.com>
Co-authored-by: DenverCoder1 <jonah@freshidea.com>
@timagixe timagixe force-pushed the issue-432-do-not-add-advanced-prop-on-submit branch from 50773aa to 736b022 Compare January 30, 2023 17:50
@timagixe
Copy link
Contributor Author

@DenverCoder1 thank you for the detailed feedback! 🚀 I appreciate that.

I skipped adding type to:

  1. copy button - since it is not associated with any form
  2. permalink button - since it will prevent it from opening the page with all the props passed in URL

Let me know what you think.

@DenverCoder1 DenverCoder1 temporarily deployed to github-readm-issue-432--959ybz January 30, 2023 18:56 Inactive
Copy link
Owner

@DenverCoder1 DenverCoder1 left a comment

Choose a reason for hiding this comment

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

Thanks for your continued contributions, looks great 🎉

@DenverCoder1 DenverCoder1 merged commit b2acf2c into DenverCoder1:main Jan 30, 2023
@DenverCoder1 DenverCoder1 changed the title Do not add advanced prop on submit fix: Do not add advanced prop on submit Jan 30, 2023
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.

Every time form is submitted using enter new advanced property with red color is added
2 participants