Simplify development of creation forms #2235
Conversation
jeremyevans
left a comment
jeremyevans
left a comment
There was a problem hiding this comment.
I like the idea/approach. I have a few inline comments/recommendations.
I tried running this branch locally and I couldn't get any of the forms using this to work correctly, but maybe I'm not running something I need to (I did regenerate the CSS using npm run prod):
issue-2235.pdf I'm guessing this is a local issue, so if you have any ideas what I missed, please let me know.
As a general comment, I recommend that methods related to UI generation be added to Clover, and not the related model classes. This will soon be important for authorization-level changes (as I mention in one comment), but in general I think UI/authorization methods should be at the web app level and not the model level.
byucesoy
force-pushed
the
creation-form-rendering
branch
from
de0b22d to
b557793
Compare
byucesoy
force-pushed
the
creation-form-rendering
branch
3 times, most recently
from
75aaa97 to
1de8a7e
Compare
|
I merged #2210, so this can be rebased to fix the tests. I'm not sure if I'm doing something wrong, but I cannot get this (1de8a7e) to work locally without changes. Looking at the Javascript, diff --git a/views/components/form/resource_creation_form.erb b/views/components/form/resource_creation_form.erb
index b2554dd4..9fc8ac7f 100644
--- a/views/components/form/resource_creation_form.erb
+++ b/views/components/form/resource_creation_form.erb
@@ -8,7 +8,7 @@ var option_dirty = <%==JSON.generate(form_elements.map { _1[:name] }.each_with_o
</script>
<div class="grid gap-6">
- <form action="<%= action %>" method="POST">
+ <form action="<%= action %>" method="POST" id="creation-form">
<%== csrf_tag(action) %>
<div class="overflow-hidden rounded-lg shadow ring-1 ring-black ring-opacity-5 bg-white">
<div class="px-4 py-5 sm:p-6">
The HTML class used for most inputs is diff --git a/package.json b/package.json
index 8eb88f26..313a6b79 100644
--- a/package.json
+++ b/package.json
@@ -8,9 +8,9 @@
"tailwindcss": "^3.4.14"
},
"scripts": {
- "dev": "npx tailwindcss -o assets/css/app.css",
- "prod": "npx tailwindcss -o assets/css/app.css --minify",
- "watch": "npx tailwindcss -o assets/css/app.css --watch"
+ "dev": "npx tailwindcss -o assets/css/app.css -i assets/css/tailwind.css",
+ "prod": "npx tailwindcss -o assets/css/app.css -i assets/css/tailwind.css --minify",
+ "watch": "npx tailwindcss -o assets/css/app.css -i assets/css/tailwind.css --watch"
},
"engines": {
"node": "22.9.0",@byucesoy, am I doing something wrong here? Does the CSS and Javascript work correctly for you when run locally without the above changes? |
jeremyevans
left a comment
jeremyevans
left a comment
There was a problem hiding this comment.
I added a couple comments with diffs, but since that may be difficult to work with, I added a branch with my changes: https://github.com/ubicloud/ubicloud/tree/jeremy-creation-form-rendering
byucesoy
force-pushed
the
creation-form-rendering
branch
from
1de8a7e to
1dc0869
Compare
|
You are right. In my local development environment, I have a script that starts the web server, which has About the |
byucesoy
force-pushed
the
creation-form-rendering
branch
2 times, most recently
from
515dc3f to
a42ddfa
Compare
|
Apologies, I did not have to time review this today as I spent all day trying to get Rails 8/Kamal 2 deploying to Ubicloud (eventually successfully). I will review this tomorrow morning. |
jeremyevans
left a comment
jeremyevans
left a comment
There was a problem hiding this comment.
All changes look good!
byucesoy
force-pushed
the
creation-form-rendering
branch
from
a42ddfa to
e0e98a6
Compare
byucesoy
force-pushed
the
creation-form-rendering
branch
from
e0e98a6 to
44a759f
Compare
We puts lots of tailwind classes in the radio_small_cards component. It makes reading the ERB code harder. In the upcoming commits, we will add branching where we would need to duplicate the tailwind classes. To make the code more readable and maintainable, I'm extracting the tailwind classes to a custom class.
radio_small_cards form component was only handling single input. In the code there are few cases where we needed to display multiple texts and for those we had to inline the content of the radio_small_cards and extend instead of directly using the component. This commit modifies the component to handle array input and display multiple texts. Co-authored-by: [email protected]
Previously, caller would send the selected value to the checkbox component and if that matched the value of the checkbox, it would be checked by default. However, this is not always the desired behavior. For example, if the caller is adding the checkbox as disabled, it should not be checked by default. Another case is that we can create multiple checkboxes with same value but different parent options. In such cases, only the checkbox whose all of the parents are checked should be checked by default. This information is not available to the checkbox component and hence it should not make the decision of checking the checkbox by default. Instead, caller can add the `checked` attribute to the checkbox component if necessary.
This aligns the behavior with the radio form element and simplifies the javascript code we need to write to handle hiding and displaying the correct form elements.
We show and hide various elements in the form based on the user's actions. However we always want the placeholder to be visible, so we add a special class called always-visible class to it, which will be used in javascript to ensure the placeholder is always visible.
When we introduce the sticky header, we unintentionally caused header to be slightly cut off. This commit fixes that by adding a margin to the top of the form.
To simplify the creation of dependency trees for options, we are introducing a new class; OptionTreeGenerator. It provides a very simple interface. You can call add_option to add a new option and if you specify a parent, it will be added as a child option of that parent. At the end, you can call serialize, which returns 2 useful constructs; a dependency tree with all possible options and paths from parents to reach specified option node, both are used in the frontend to render the form.
This code was not being used.
Previously, caller would send the selected value to the radio input component and if that matched the value of the radio input, it would be selected by default. However, this is not always the desired behavior. For example, if the caller is adding the radio as disabled, it should not be selected by default. Another case is that we can create multiple radio inputs with same value but different parent options. In such cases, only the radio input whose all of the parents are selected should be selected by default. This information is not available to the radio component and hence it should not make the decision of selecting the radio input by default. Instead, caller can add the `checked` attribute to the radio component if necessary.
This simply renders title and content for new section. It will be used for load balancer creation form.
This will be used by PostgreSQL creation form.
This commit itself adds a seemingly simple component, which iterates over form components and renders them. However, its simplicity is thanks to alignments we made in previous commits and the modularity we are adding in future commits.
We use strict CSP headers in our application, which prevents running any inline scripts. However, we need to pass big JSON data from backend to frontend for dynamically show/hide form options. This commit adds script tag with nonce to enabling inline script running only for the script we are generating from the backend.
Previously, the assets/css/tailwind.css had no effect. I guess previously, it didn't matter, but now that radio-small-cards relies on assets/css/tailwind.css, we need to use it so the CSS includes the custom Tailwind components.
byucesoy
force-pushed
the
creation-form-rendering
branch
from
44a759f to
545115a
Compare
Problem:
Developing resource creation forms is quite troublesome especially if the options and prices change based on previously selected options. One extreme example is PostgreSQL creation form.
So far, we implemented custom javascript logic for each special case, but as the number of cases increase, it become unfeasible. Breaking point for me was needing to implement UI for PostgreSQL scale up/down, which needed entirely new custom logic because it was difficult to change existing logic that completely depends location being at the top of the hierarchy, whereas for scale up/down there wouldn't be location selection. Similar problem exists in other resource forms as well, so I wanted to simplify overall development with this patch.
Solution:
Core of the problem was both frontend and backend tries to handle both which child options are available for selected parent options and also the overall layout. The solution was splitting the responsibilities. Backend will be responsible for deciding which child options are available for selected parent options and the frontend will be responsible for the layout.
For this purpose, backend sends an
option_treeto frontend, which has branches for all possible user selections. Here is an exampleoption_tree. As you can seename, which refers tonametextbox is standalone leaf node, whereasstorage_sizeis child ofsize, which is child oflocation.If you look at it closely, you will see there is a lot of repetition, especially for leaf nodes. For example there are many
storage_size=512options. This might seem unnecessary, but it isn't, becausestorage_size=512might be valid option forlocation=eu-central-h1, size=standard-2, but it is not valid options forlocation=us-east-a2, size=standard-2. So we need to putstorage_size=512multiple times to the branches where it is a valid option.Even if they are necessary, generating this tree with all possible options requires some work. Hence, I introduce a new class
OptionTreeGenerator, which makes generating this tree a breeze. Here is how this tree is generated;You just need to add a new option and specify its parent (if it has one). Optionally you can pass a block, which would specifies given parent and child values, whether the option is valid one.
On the frontend side, you need to specify the layout, which is done by defining form_elements array;
ContentGeneratorpart might seem scary. It is not. It is just for generating proper display text.Then What?:
Then form will be rendered properly and more importantly, depending on what user select, correct sub options will be displayed recursively. No need to write javascript code, no need to write form code, no need to write component code. You just specify the layout and call
render("components/form/resource_creation_form").PS: Failed tests will be fixed by #2210.