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

Add requireness to HasOne form #2812

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

Conversation

goosys
Copy link
Contributor

@goosys goosys commented Mar 17, 2025

The nested form for HasOne didn't have the requiredness mark, so I added it.

Before:

image

After:

image

@nickcharlton
Copy link
Member

Can you think of a reasonable way to test this, so we don't break it in future?

@goosys
Copy link
Contributor Author

goosys commented Mar 20, 2025

I will think of a reasonable way to test this and add it!

@goosys
Copy link
Contributor Author

goosys commented Mar 24, 2025

I've added tests.

By the way, ProductoMetaTag is the only place in the demo app that uses Field::HasOne, but it has too few columns, both of which are String type and required. This makes it less suitable for comprehensive testing.

Would you consider adding more columns?

@goosys
Copy link
Contributor Author

goosys commented Mar 24, 2025

Regarding this method name, is "requireness" correct? Should it be "requiredness" instead?

@nickcharlton
Copy link
Member

By the way, ProductoMetaTag is the only place in the demo app that uses Field::HasOne, but it has too few columns, both of which are String type and required. This makes it less suitable for comprehensive testing.

We should add more if it's needed for testing. The demo app has grown organically and is neither consistent nor coherent in how it works. I've long wanted to switch the demo app to follow a particular pattern. Maybe a shop or something.

Regarding this method name, is "requireness" correct? Should it be "requiredness" instead?

I think "requiredness" is likely more correct, according to the OED it is a word, although not a common one.

@goosys goosys force-pushed the fix-has_one-requireness branch from db181cc to 0d833b1 Compare March 27, 2025 05:43
@goosys
Copy link
Contributor Author

goosys commented Mar 27, 2025

Rebased!

@@ -27,7 +27,7 @@ The form will be rendered as nested_from to parent relationship.
<% end %>

<% attributes.each do |attribute| %>
<div class="field-unit field-unit--<%= attribute.html_class %>">
<div class="field-unit field-unit--<%= attribute.html_class %> field-unit--<%= requireness(attribute) %>">
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
<div class="field-unit field-unit--<%= attribute.html_class %> field-unit--<%= requireness(attribute) %>">
<div class="field-unit field-unit--<%= attribute.html_class %> field-unit--<%= requiredness(attribute) %>">

I think we should probably make the change to "requiredness", as I do think that's more correct than "requireness".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should this be changed in this PR?
Or would it be okay if I open a new PR?

Copy link
Member

Choose a reason for hiding this comment

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

Oh! Right, it's not actually introduced here. I hadn't realised. Let's leave it for now, we've already got the existing name and it's fine.

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