-
-
Notifications
You must be signed in to change notification settings - Fork 7k
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 batch actions and categories to admin UI for custom emojis #11793
Conversation
|
2f4f79c
to
137ef4e
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 overall, but see inline comments
.fields-group.fields-row__column.fields-row__column-6 | ||
.input.string.optional | ||
.label_input | ||
= f.text_field :category_name, class: 'string optional', placeholder: t('admin.custom_emojis.create_new_category'), 'aria-label': t('admin.custom_emojis.create_new_category') |
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 those would be better as a single field with autocompletion… that can be left for later though.
redirect_to admin_custom_emojis_path(page: params[:page], **@filter_params) | ||
def batch | ||
@form = Form::CustomEmojiBatch.new(form_custom_emoji_batch_params.merge(current_account: current_account, action: action_from_button)) | ||
@form.save |
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.
Shouldn't we have a flash message to tell the changes were processed?
expect(response).to redirect_to admin_custom_emojis_path | ||
expect(custom_emoji.reload).to have_attributes(disabled: true) | ||
end | ||
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.
Specs that cover the same use case with the new actions should probably be written.
137ef4e
to
3457c1f
Compare
Fix #11402