-
Notifications
You must be signed in to change notification settings - Fork 20
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 configuration to "Add another" #4575
Conversation
f172a68
to
865981f
Compare
b78db53
to
547f2d8
Compare
547f2d8
to
b9bfd0c
Compare
27b3638
to
d1b963a
Compare
d1b963a
to
e2013b6
Compare
e2013b6
to
8f7893b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, just had a couple questions 👍
<%= item[:destroy_checkbox] %> | ||
</div> | ||
<%= item[:fields] %> | ||
<div data-module="add-another" class="gem-c-add-another" data-add-button-text="<%= add_button_text %>" data-fieldset-legend="<%= fieldset_legend %>" data-empty-fields="<%= empty_fields %>"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think at this point it may be better to use the built in Rails tag.div
syntax for adding data attributes e.g. <%= tag.div class: "classes", data: { fieldset_legend: "etc" } do %>
render_component({ items: default_items, empty_fields: true }) | ||
|
||
assert_select "div[data-module='add-another'][data-empty-fields='true']" | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does the behaviour need to be tested e.g. it renders the empty state correctly?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point: I've updated these tests a bit to duplicate and change them for the "Default" and "Start empty" options, which is probably the clearest way to present what it being tested.
8f7893b
to
dba9a8c
Compare
321a551
to
09a891c
Compare
Thanks @AshGDS - I've made a couple of updates in resonse to your queries if you'd be able to hae another quick look:
|
module: "add-another", | ||
"add-button-text": add_button_text, | ||
"fieldset-legend": fieldset_legend, | ||
"empty-fields": empty_fields |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you can forgo using strings as Ruby would convert underscores to hyphens (e.g. add_button_text
would become data-add-button-text
, but this isn't a massive issue of course so feel free to leave it!
- Add new "start_empty" option to component doc - Hidden class on "Delete" button added conditionaly on `empty-fields` data attribute - Add `empty-fields` data-attribute to view - Update JS test - Update RSpec test - Update CHANGELOG
09a891c
to
95f291a
Compare
What
Publishing Components Issue #4564
This adds some configuration to the "Add another" component so that authors can choose how the component displays by default when it is rendered on a page and no content is provided to it.
Why
The current default behaviour of the "Add another" component when it loads is to:
There is a need to customise this behaviour for these scenarios when it is implemented on the Mainstream publisher app so that:
Visual Changes
The screenshots below show the component implemented on Mainstream Publisher as it currently displays and how it is displayed with these changes.