gpadvs-dropdown-remove-button.js: Added snippet to show remove button on Dropdown fields.
#1028
Conversation
veryspry
force-pushed
the
matt/gpadvs-x
branch
from
80dcfef to
793d79e
Compare
…scope to _only_ `dropdown` or `multiselect` fields.
WalkthroughTwo new JavaScript files have been added to the GP Advanced Select module. Each file registers a new filter via Changes
Sequence Diagram(s)sequenceDiagram
participant User as End User
participant GF as Gravity Forms
participant ClearFilter as Clear Button Filter (gpadvs-clear-button.js)
participant TS as Tom Select
User->>GF: Load Advanced Select Field (Dropdown)
GF->>ClearFilter: Invoke gform.addFilter('gpadvs_settings')
ClearFilter-->>GF: Return settings with clear_button plugin enabled
GF->>TS: Initialize Advanced Select with clear button
User->>TS: Interact with clear button
sequenceDiagram
participant User as End User
participant GF as Gravity Forms
participant RemoveFilter as Remove Button Filter (gpadvs-dropdown-remove-button.js)
participant TS as Tom Select
User->>GF: Load Advanced Select Field (Multi-Select)
GF->>RemoveFilter: Invoke gform.addFilter('gpadvs_settings')
RemoveFilter-->>GF: Return settings with remove_button plugin enabled
GF->>TS: Initialize Advanced Select with remove button
User->>TS: Interact with remove button
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (1)
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
@saifsultanc good call! I added a conditional along with a comment detailing how this can be applied to specific field type or all GPADVS fields: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
gp-advanced-select/gpadvs-clear-button.js (3)
5-5: Fix the typo in the documentation.There's a typo in "Clear buton" - it should be "Clear button".
- * Adds a Clear buton to GP Advanced Select fields. + * Adds a Clear button to GP Advanced Select fields.
5-7: Improve documentation clarity.The documentation could be more specific about which field types this applies to by default, similar to how it's described in the dropdown-remove-button file.
* Adds a Clear button to GP Advanced Select fields. + * By default, this is only applied to Dropdown fields. * * The Clear Button is a built in plugin of Tom Select.
24-26: Consider i18n for the button title.The title "Clear options" is hardcoded, which might not be ideal for internationalization. Consider using a similar approach as in the remove-button implementation where it checks for localized strings first.
settings.plugins.clear_button = { - title: 'Clear options', + title: window.GPADVS.strings?.clear_options + ? window.GPADVS.strings.clear_options + : 'Clear options', };gp-advanced-select/gpadvs-dropdown-remove-button.js (1)
19-23: Add safety check for window.GPADVS.While you're using optional chaining for
strings?.remove_this_item, it's good practice to also check ifwindow.GPADVSexists to prevent potential errors.settings.plugins.remove_button = { - title: window.GPADVS.strings?.remove_this_item - ? window.GPADVS.strings.remove_this_item + title: window.GPADVS?.strings?.remove_this_item + ? window.GPADVS.strings.remove_this_item : 'Remove this item', }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
gp-advanced-select/gpadvs-clear-button.js(1 hunks)gp-advanced-select/gpadvs-dropdown-remove-button.js(1 hunks)
🔇 Additional comments (2)
gp-advanced-select/gpadvs-clear-button.js (1)
15-31: Implementation looks good!The implementation correctly adds the clear button plugin to dropdown fields only, with helpful comments about how to adjust the target field types if needed.
gp-advanced-select/gpadvs-dropdown-remove-button.js (1)
16-27: Consistent i18n implementation is good.The implementation using a configurable title via
window.GPADVS.stringsis good for internationalization, similar to how one would implement localization in WordPress.
| * Adds a Remove button to selection options in Dropdown fields. By default, | ||
| * the remove button is only added to Multi-Select fields. | ||
| * |
There was a problem hiding this comment.
Documentation and implementation mismatch.
The documentation states this adds a Remove button to Dropdown fields and is only added to Multi-Select fields by default. However, the implementation applies to all field types without any conditional check (unlike the clear-button implementation).
Either update the documentation to match the implementation or add a conditional check similar to the clear-button implementation:
function(settings, gpadvsInstance, selectNamespace) {
+ /**
+ * Scope to only multi-select fields.
+ * This conditional can be modified to target different field types.
+ */
+ if (gpadvsInstance.fieldType === 'multiselect') {
settings.plugins.remove_button = {
title: window.GPADVS.strings?.remove_this_item
? window.GPADVS.strings.remove_this_item
: 'Remove this item',
}
+ }
return settings;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| * Adds a Remove button to selection options in Dropdown fields. By default, | |
| * the remove button is only added to Multi-Select fields. | |
| * | |
| function(settings, gpadvsInstance, selectNamespace) { | |
| /** | |
| * Scope to only multi-select fields. | |
| * This conditional can be modified to target different field types. | |
| */ | |
| if (gpadvsInstance.fieldType === 'multiselect') { | |
| settings.plugins.remove_button = { | |
| title: window.GPADVS.strings?.remove_this_item | |
| ? window.GPADVS.strings.remove_this_item | |
| : 'Remove this item', | |
| } | |
| } | |
| return settings; | |
| } |
There was a problem hiding this comment.
Nice one, code rabbit :finger-guns. Updated ✅
There was a problem hiding this comment.
@veryspry Thank you for implementing the suggested change! This will ensure the implementation aligns with the documentation and targets only multi-select fields as intended.
(\ /)
(･ᴗ･)
/ >🥕
Context
⛑️ Ticket(s): https://secure.helpscout.net/conversation/2861069601/78654?viewId=6987275
Summary
Adds the "remove button" plugin for Dropdown fields.