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

Refactor form-helper functions into more streamlined API #106

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

Khachig
Copy link

@Khachig Khachig commented Aug 4, 2023

Simplifying the function APIs

I've refactored the form-helper functions into a simpler — in my opinion — API to avoid passing what I thought was a needless dictionary and key to the functions. I think this refactor removes some redundancy from the API.

Currently

Most of the time, I want to create a text input form with the name and placeholder attributes, along with some classes. In order to do this currently, I would have to pass in the required struct but keep it empty since I don't want to set a default value attribute.

(text-field {} :textfield :placeholder "Enter text" :class "my-class")
# => <input type="text" name="textfield" placeholder="Enter text" class="my-class" />

If I do want to add a default value, I have two ways to do it:

  1. Using the struct (thereby also duplicating the key)
(text-field {:textfield "Default value"} :textfield :placeholder "Enter text" :class "my-class")
# => <input type="text" name="textfield" value="Default value" placeholder="Enter text" class="my-class" />
  1. Using the default HTML value attribute (still have to pass an empty struct)
(text-field {} :textfield :value "Default value" :placeholder "Enter text" :class "my-class")
# => <input type="text" name="textfield" value="Default value" placeholder="Enter text" class="my-class" />

I think we can do away with this redundancy since there's already a default way to define the default value of HTML elements. Another issue is that the val dictionary being passed into the field function is only used to retrieve the value of the field with the same key in the second argument, which makes any other fields in the dictionary pointless to the construction of the input element.

Whatever slight benefit that could be gained by reusing a table of attributes for the sole purpose of extracting the value attribute is outweighed by the redundant API. Allowing the first method also opens up the possibility for inconsistency in the codebase if the same thing is done differently in different places. Pre-filling input fields is also not the desired default behavior in most cases for non-hidden inputs so an empty struct would have to be passed in most of the time.

After this refactor

By default, attributes other than name are not set unless done explicitly. There is also no need to pass a dictionary and duplicate the key but simply a keyword or string for the name attribute and the optional attributes after that. I believe this makes for a much simpler and intuitive API. Here's the code to do the same thing as above.

(text-field :textfield :placeholder "Enter text" :class "my-class")
# => <input type="text" name="textfield" placeholder="Enter text" class="my-class" />

Khachig added 2 commits August 4, 2023 12:51
Refactored form-helper functions into a simpler API to avoid
verbose argument lists with dictionaries and keys. Also updated
docstrings to reflect the new function APIs.
@swlkr
Copy link
Collaborator

swlkr commented Aug 16, 2023

Oh I thought I replied to this.

This is a good change, I guess my only hang up is that if anyone is out there using this, when they upgrade it's going to break them.

Is there a way we can make this backwards compatible?

Maybe check the number of arguments in the function or come up with new names text-input instead of text-field or my last idea would be to come up with a new module form-helper2 or something and people can opt in to the better api?

@Khachig
Copy link
Author

Khachig commented Oct 16, 2023

Apologies for disappearing. Life gets in the way sometimes. Just wanted to say that I agree with your comment and I'll make an update soon.

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.

2 participants