Replace Select2 with SlimSelect #448

gmq · 2023-03-06T18:49:03Z

Context

Select2 is difficult to configure and use with newer bundlers based on esbuild. Rather than attempting to make it work, this pull request replaces the library with a more up-to-date alternative while attempting to maintain similar usage.

Breaking Changes

This implementation replaces ActiveAdmin's jQuery events with a MutationObserver. Instead of listening to has_many_* events, the MutationObserver watches for new elements and initializes slim-select where necessary. It covers most typical use cases, but there may be scenarios that I have not taken into account.

Changes

I added our standard ESLint configuration to accommodate the use of ES6 syntax in newer files. This may result in a lot of red lines in older files, but since they are not touched often, it shouldn't be a major issue.

Possible Issues

It may be necessary to fork slim-select to fix outstanding issues since the author of the library works sporadically on it. This is alleviated in part by the fact that we don't really need any new features. Once we get it working with no issues, there won't be a need for continued development, and we can contribute the fixes to upstream.

monkeyci · 2023-03-06T18:55:07Z

lib/activeadmin_addons.rb

@@ -4,7 +4,7 @@ module ActiveadminAddons
  attr_writer :default_select, :datetime_picker_default_options, :datetime_picker_input_format

  def default_select
-    return "select2" unless @default_select
+    return "slim-select" unless @default_select


[rubocop] _{reported by reviewdog 🐶}
Add empty line after guard clause.

monkeyci · 2023-03-06T18:55:08Z

spec/features/inputs/nested_select_input_spec.rb

-        on_input_ctx("invoice_region_id") { select2_options_container.click }
-        on_input_ctx("invoice_city_id") { open_select2_options }
-        expect_select2_result_text_to_eq(1, msg)
+        msg = "Input is too short"


⚠️ [rubocop] _{reported by reviewdog 🐶}
Useless assignment to variable - msg.

monkeyci · 2023-03-06T18:55:08Z

spec/features/inputs/slimselect_spec.rb

    end
  end

-  context "when default config is Active Admin's default and select control has select 2 class" do
+  context "when default config is Active Admin's default and select control has slim-select class" do


[rubocop] _{reported by reviewdog 🐶}
Line is too long. [101/100]

monkeyci · 2023-03-06T18:55:08Z

spec/features/inputs/tags_input_spec.rb

-        fill_select2_input_and_press_return("Not preloaded item")
-        expect_select2_choices_count_to_eq(1)
+        expect_slimselect_options_count_to_eq(3)
+        expect { add_slimselect_option("Not preloaded item") }.to raise_error(Capybara::ElementNotFound)


[rubocop] _{reported by reviewdog 🐶}
Line is too long. [104/100]

monkeyci · 2023-03-06T18:55:09Z

spec/support/capybara_helpers.rb

  end

-  def select2_input
-    find(".select2-search__field")
+  def expect_slimselect_options_count_to_eq(count, id=nil)


[rubocop] _{reported by reviewdog 🐶}
Surrounding space missing in default value assignment.

monkeyci · 2023-03-06T18:55:09Z

spec/support/capybara_helpers.rb

-  def select2_input
-    find(".select2-search__field")
+  def expect_slimselect_options_count_to_eq(count, id=nil)
+    id = slimselect_original_select_id unless id


[rubocop] _{reported by reviewdog 🐶}
Use the double pipe equals operator ||= instead.

monkeyci · 2023-03-06T18:55:09Z

spec/support/capybara_helpers.rb

-  def expect_select2_options_count_to_eq(count)
-    expect(page).to have_css("select.select2-hidden-accessible option", count: count)
+  def pick_slimselect_entered_option(item_text, display_name = nil)
+    display_name = item_text unless display_name


[rubocop] _{reported by reviewdog 🐶}
Use the double pipe equals operator ||= instead.

monkeyci · 2023-03-06T18:55:09Z

spec/support/capybara_helpers.rb

+  end
+
+  def expect_slimselect_popover_is_closed(id = nil)
+    expect(page.document).not_to have_css(".ss-content[data-id=#{id || slimselect_original_select_id}]")


[rubocop] _{reported by reviewdog 🐶}
Line is too long. [104/100]

difernandez

Nice! Like how the js of different slim select input types is organized 💯. I leave some comments, let me know what you think. I only reviewed the code, when I have some time I'll do some testing in local and I'll come back if I find something

difernandez · 2023-03-07T15:19:09Z

package.json

+    "eslint-plugin-import": "^2.26.0",
+    "eslint-plugin-platanus": "^0.1.0",


I think this should also go in devDependencies

difernandez · 2023-03-07T15:49:50Z

app/inputs/active_admin/inputs/select_input.rb

@@ -1,6 +1,10 @@
 class ActiveAdmin::Inputs::SelectInput < Formtastic::Inputs::SelectInput
  def input_html_options
-    super.merge(data: { tags: @options[:tags].present? })
+    if @options[:tags].present?
+      super.deep_merge(data: { tags: true }, class: 'simple-tags-input')


Is the data: { tags: true } necessary now? It was added in this PR because select2 received options via data-* attributes, not sure if slim select needs it

difernandez · 2023-03-07T15:53:37Z

app/javascript/activeadmin_addons/addons/slim-select-interactive-tag.js

+}
+
+function setupSelect(el) {
+  // eslint-disable-next-line no-new


there's no new in the next line, don't think this is necessary

difernandez · 2023-03-07T16:03:34Z

app/assets/javascripts/activeadmin_addons/all.js

@@ -2,11 +2,10 @@
 /**
 * Warning: This file is auto-generated, do not modify. Instead, make your changes in 'app/javascript/activeadmin_addons/' and run `yarn build`
 */
-//= require select2.full


we have a select2.full.js and select2.css in our vendor, should we also remove it?

difernandez · 2023-03-07T19:30:52Z

app/javascript/activeadmin_addons/inputs/slim-select-selected-list.js

+        // eslint-disable-next-line max-statements
+        newVal.forEach((data) => {
+          const itemId = `${prefix}_${data.value}`;
+          if (document.querySelectorAll(`#${itemId}`).length > 0) {


we could use querySelector to not gather all of them

!!document.querySelector(`#${itemId}`)

difernandez · 2023-03-07T19:40:22Z

app/javascript/activeadmin_addons/inputs/slim-select-tags.js

+    // eslint-disable-next-line no-unused-vars
+    const { id, ...rest } = item;
+
+    return { ...rest, value: item.id, selected: !!item.selected };


you could use id instead of item.id, and avoid the eslint-disable a couple lines before

difernandez · 2023-03-07T19:42:55Z

app/javascript/activeadmin_addons/inputs/slim-select-tags.js

+          hiddenInput.name = itemName;
+          hiddenInput.value = data.value;
+          hiddenInput.type = 'hidden';
+          hiddenInput.textContent = data.text;


not sure, but is it okay for the input to have textContent?

It appends the text next to the input, but I can't remember why I added it so I removed it.

difernandez · 2023-03-07T19:58:50Z

app/javascript/activeadmin_addons/inputs/slim-select-utils.js

+  return queryArray.join('&');
+}
+
+// eslint-disable-next-line import/prefer-default-export


We recently changed this rule to prefer named exports in Potassium, maybe we should update it here too

ldlsegovia

I have manually tested the functionality in the Dummy app and I found these details:

Indentation:
before:

after:
Cleaning button
before:

after:
Red border:
before:

after:
Clean value after selecting
before:

after:

app/inputs/nested_level_input.rb

difernandez

One small detail, otherwise it's looking pretty good to me 👌I'll try the branch later today to see if there's anything else. One other thing I just remembered: we should update the docs, there are various mentions of select2 in there, as well as filenames like select2_search.md

spec/features/inputs/tags_input_spec.rb

difernandez

Found a couple things while testing this:

Display text is being interpreted as html. The part between <> dissapears and is an html tag in devtools

Screen.Recording.2023-03-16.at.11.49.57.mov

Interactive tag is not working for me 🤔 select appears and disappears immediately

Screen.Recording.2023-03-16.at.11.56.42.mov

ldlsegovia

Excellent! The only issue that remains is the indentation problem. Could you review that please? :)

difernandez

excellent! 👏 🚀

ldlsegovia

👏🏻

gmq requested review from ldlsegovia and difernandez March 6, 2023 18:49

monkeyci reviewed Mar 6, 2023

View reviewed changes

difernandez suggested changes Mar 7, 2023

View reviewed changes

ldlsegovia reviewed Mar 7, 2023

View reviewed changes

app/inputs/nested_level_input.rb Show resolved Hide resolved

gmq requested review from ldlsegovia and difernandez March 10, 2023 15:59

difernandez suggested changes Mar 13, 2023

View reviewed changes

spec/features/inputs/tags_input_spec.rb Outdated Show resolved Hide resolved

gmq requested a review from difernandez March 13, 2023 19:34

gmq added 20 commits March 14, 2023 10:58

chore(): add eslint

65a7f7a

chore(js): remove select2 and install slim-select

ce0d71b

feat(slim-select): add base handler

60392b4

chore(slim-select): replace select2 helpers with slim-select helpers

ede018a

feat(slim-select): add basic tag and search functionality

d8e895f

feat(slim-select): add nested selects

76b0685

feat(slim-select): add selected list

e4cdfe8

feat(slim-select): add tags select

4530b28

feat(slim-select): copy css to fix sprockets errors

030857a

chore(): remove remaining select2 references

a8a12f2

feat(): remove jquery from initializer

96806ea

chore(): update compiled all.js

5f0b507

chore(slim-select): update remaining tests

f68f9fe

feat(slim-select): always allow deselect

f60cb98

feat(slim-select): add red border on error

f029380

feat(slim-select): deselect when choosing an item in a selected list

e4dd1d2

chore(select2): remove remaining assets

6bae7f3

chore(): update to use named exports

7146a3f

chore(): update slim-select

c791e90

feat(slim-select): update documentation

ebe6587

gmq force-pushed the remove-select2 branch from 872bad9 to ebe6587 Compare March 14, 2023 13:59

gmq changed the base branch from v2 to master March 14, 2023 14:00

difernandez suggested changes Mar 16, 2023

View reviewed changes

ldlsegovia reviewed Mar 16, 2023

View reviewed changes

gmq added 3 commits March 16, 2023 16:24

fixup! feat(slim-select): add tags select

77212e1

feat(slim-select): remove default width

5e1bbbf

feat(slim-select): use platanus fork

5b7307e

gmq requested review from difernandez and ldlsegovia March 16, 2023 20:59

difernandez approved these changes Mar 17, 2023

View reviewed changes

ldlsegovia approved these changes Mar 17, 2023

View reviewed changes

gmq merged commit 7614d15 into master Mar 17, 2023

gmq deleted the remove-select2 branch March 17, 2023 18:59

difernandez mentioned this pull request Apr 4, 2023

Boolean select width issue in filters #378

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace Select2 with SlimSelect #448

Replace Select2 with SlimSelect #448

gmq commented Mar 6, 2023 •

edited

Loading

monkeyci Mar 6, 2023

monkeyci Mar 6, 2023

monkeyci Mar 6, 2023

monkeyci Mar 6, 2023

monkeyci Mar 6, 2023

monkeyci Mar 6, 2023

monkeyci Mar 6, 2023

monkeyci Mar 6, 2023

difernandez left a comment

difernandez Mar 7, 2023

difernandez Mar 7, 2023

difernandez Mar 7, 2023

difernandez Mar 7, 2023

difernandez Mar 7, 2023

difernandez Mar 7, 2023

difernandez Mar 7, 2023

gmq Mar 10, 2023

difernandez Mar 7, 2023

ldlsegovia left a comment

difernandez left a comment

difernandez left a comment •

edited

Loading

ldlsegovia left a comment

difernandez left a comment

ldlsegovia left a comment

		"eslint-plugin-import": "^2.26.0",
		"eslint-plugin-platanus": "^0.1.0",

Replace Select2 with SlimSelect #448

Replace Select2 with SlimSelect #448

Conversation

gmq commented Mar 6, 2023 • edited Loading

Context

Breaking Changes

Changes

Possible Issues

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

difernandez left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ldlsegovia left a comment

Choose a reason for hiding this comment

difernandez left a comment

Choose a reason for hiding this comment

difernandez left a comment • edited Loading

Choose a reason for hiding this comment

ldlsegovia left a comment

Choose a reason for hiding this comment

difernandez left a comment

Choose a reason for hiding this comment

ldlsegovia left a comment

Choose a reason for hiding this comment

gmq commented Mar 6, 2023 •

edited

Loading

difernandez left a comment •

edited

Loading