Skip to content

Conversation

@SegiNyn
Copy link
Contributor

@SegiNyn SegiNyn commented Feb 12, 2024

This Pr adds some more options to the RemoteDropdown widget.

  • Allow selecting multiple
  • Allow additions
  • Display previously selected value
  • Allow directly passing the options

if isinstance(field.widget, TypeaheadWidget):
return True
if isinstance(field.widget, Select):
if isinstance(field.widget, (DropdownWidget, Select)):
Copy link
Member

Choose a reason for hiding this comment

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

Are you sure this is correct? We use Select here because it becomes bigger in multiple mode. But the SUI Dropdown has the same size regardless of whether it's for a single value or multiple values. So I think it should stay above in the return True case.

Copy link
Contributor Author

@SegiNyn SegiNyn Feb 12, 2024

Choose a reason for hiding this comment

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

Well I guess it doesn't make much difference:
It stays the same and the height increases as you add more items. This is the position of the label if checking multiple
Screenshot 2024-02-12 at 13 57 27
This is the position if left the way it was and return True always
Screenshot 2024-02-12 at 13 59 37

@SegiNyn SegiNyn force-pushed the sui-dropdown branch 2 times, most recently from 462e9c9 to da2a9f5 Compare February 12, 2024 13:30
fetchData();
}
}, [preload, fetchData]);
}, [preload, fetchData, optionsFromProps, transformOptions]);
Copy link
Member

Choose a reason for hiding this comment

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

FYI this only works because the component is used in a kind of special way, and optionsFromProps is always the same object (not just the same data in the object). Otherwise this would cause a render loop, since e.g. {} != {}. But since this is always populated from a statically defined object, it works fine.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh okay, got it! Good to understand more the intricacies of JS

@ThiefMaster
Copy link
Member

Now that I finally tested it (using the code at the end of this comment on /admin/announcement since that's an easy to use wtform that doesn't have dialogs or any other annoyances), I found a few things:

  • [kind of fixed] multiple and allow_additions combined lets you add e,g, abc,def (in a single item), but it gets serialized as abc,def in the submitted data so it cannot be distinguished from abc and def as separate items. I think the way the field is currently implemented is pretty much incompatible with multiple mode in general, because in that case the submitted formdata should be like name=abc&name=def and not name=abc,def. This is generally done by repeating the <input type="hidden"> once for each value, or by sending a JSON array of items (the latter will be WAY easier to implement here!) I added a commit that uses JSON for that field, but at least when using the JSONField we have this somehow breaks with the initial value (null). Maybe this should be handled more nicely on the client side since I do not want to touch JSONField (used in way too many places), or maybe one should not use the unmodified JSONField for this widget since it's always been meant to be used with a custom field class anyway.
  • [just a big ugly, not really a bug] allow_additions sends either the entered string or the id. this feels a bit ugly, but I think it's acceptable since you may want to change the config to simply not use IDs at all but always send the name (and then determine existing vs new on the backend)
  • [TODO] the size of the multiple field looks weird, it's especially noticeable when you added one item and then start adding another one: image
  • [weird behavior, fixed] clearing a multi field makes no sense in most cases (you can already remove individual entries), so I disabled this
  • [already fixed] there was a bug with additions: add something new (e.g. "test") to the field, then start typing something in the field again --> TypeError: opt.search is undefined
diff --git a/indico/modules/announcement/controllers.py b/indico/modules/announcement/controllers.py
index 99f6236dac..00b397b497 100644
--- a/indico/modules/announcement/controllers.py
+++ b/indico/modules/announcement/controllers.py
@@ -20,7 +20,9 @@ class RHAnnouncement(RHAdminBase):
     def _process(self):
         form = AnnouncementForm(obj=FormDefaults(**announcement_settings.get_all()))
         if form.validate_on_submit():
-            announcement_settings.set_multi(form.data)
-            flash(_('Settings have been saved'), 'success')
+            print('test', form.test.data)
+            print('test2', form.test2.data)
+            # announcement_settings.set_multi(form.data)
+            # flash(_('Settings have been saved'), 'success')
             return redirect(url_for('announcement.manage'))
         return WPAnnouncement.render_template('settings.html', 'announcement', form=form)
diff --git a/indico/modules/announcement/forms.py b/indico/modules/announcement/forms.py
index ebc51cd388..b14438606d 100644
--- a/indico/modules/announcement/forms.py
+++ b/indico/modules/announcement/forms.py
@@ -5,16 +5,27 @@
 # modify it under the terms of the MIT License; see the
 # LICENSE file for more details.

+from wtforms import StringField
 from wtforms.fields import BooleanField, TextAreaField
 from wtforms.validators import DataRequired

 from indico.util.i18n import _
 from indico.web.forms.base import IndicoForm
 from indico.web.forms.validators import UsedIf
-from indico.web.forms.widgets import SwitchWidget
+from indico.web.forms.widgets import DropdownWidget, SwitchWidget


 class AnnouncementForm(IndicoForm):
     enabled = BooleanField(_('Enabled'), widget=SwitchWidget())
     message = TextAreaField(_('Message'), [UsedIf(lambda form, _: form.enabled.data), DataRequired()],
                             description=_('You may use Markdown and basic HTML elements for formatting.'))
+    test = StringField('Additions', widget=DropdownWidget(choices=[
+        {'id': 12, 'name': 'foo'},
+        {'id': 34, 'name': 'bar'},
+        {'id': 56, 'name': 'foobar'},
+    ], allow_additions=True, multiple=False))
+    test2 = StringField('Additions+Multiple', widget=DropdownWidget(choices=[
+        {'id': 12, 'name': 'foo'},
+        {'id': 34, 'name': 'bar'},
+        {'id': 56, 'name': 'foobar'},
+    ], allow_additions=True, multiple=True))

@ThiefMaster
Copy link
Member

@SegiNyn Please have a look at the display issue from my previous comment, and look over my other comments and the fixes I made. I think if you don't spot any problems with them and fix the display problem, we can merge this.

@OmeGak
Copy link
Member

OmeGak commented Feb 16, 2024

I tested with the diff shared by @ThiefMaster and found a couple of corner cases.


When allow_additions is enabled it always propose to add the item, even if it's already present in the dropdown. I would expect the "Add X" to appear only if it matches none of the items.

image

With multiple enabled, hovering the tag changes its color and turns the cursor into a pointer. This conveys that the tag can be interacted with but clicking yields no result. I believe that only the cross should change the cursor to a pointer. The screenshot below shows the different color when hovering the third tag (cursor is not caught by the screenshot).

image

@SegiNyn
Copy link
Contributor Author

SegiNyn commented Feb 16, 2024

Hi @ThiefMaster,

  1. Fixed the styles of the multiple appearing weird
  2. For the issue @OmeGak mentioned about choosing multiple, the reason was because of the search highlighting. Basically a different object is created when the text is transformed to be highlighted and the dropdown did not see them as the same options which is why it was possible to add duplicates
  3. Changed to use the default cursor when hovering on the multiple selected item, so it's now only a pointer when on the close button.

@SegiNyn
Copy link
Contributor Author

SegiNyn commented Feb 16, 2024

Maybe you might have a different opinion on what to do with the highlighting, but what I've done is to disable the highlight when allowAdditions is True

@ThiefMaster
Copy link
Member

For me it's fine. The styling is still a tiny bit strange in case of multiple mode (more space below than above), but I'd be OK merging it now (but if you want to tweak this more let me know and I'll wait)

image

@SegiNyn
Copy link
Contributor Author

SegiNyn commented Feb 20, 2024

Okay let me see if I can tweak it some more.

@ThiefMaster
Copy link
Member

Thanks! FYI I just pushed the latest rebased version (no changes besides rebasing)

@SegiNyn
Copy link
Contributor Author

SegiNyn commented Feb 20, 2024

@ThiefMaster, I've removed the space below so it looks like this. But about the highlighting, do you think there is something else we can do in the filtering options that would allow the search to still be highlighted but not treated as a different entry when allowadditions is enabled?
Screenshot 2024-02-20 at 16 06 56
(Btw I'm using unique values but same text in the choices in the image so they are not duplicates)

// LICENSE file for more details.

:global(.ui.multiple.search.dropdown) :global(input[type='text'].search) {
margin: 0em 0.64285714em;
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
margin: 0em 0.64285714em;
margin: 0 0.64285714em;

0 usually needs no unit

@ThiefMaster
Copy link
Member

do you think there is something else we can do in the filtering options that would allow the search to still be highlighted but not treated as a different entry when allowadditions is enabled

TBH I'm not really worried about not having the highlighting there. So if someone later wants to improve this fine, but it's not something I'll spend time on for now ;)

@ThiefMaster ThiefMaster enabled auto-merge (squash) February 20, 2024 15:40
@ThiefMaster ThiefMaster added this to the v3.3 milestone Feb 20, 2024
@ThiefMaster ThiefMaster merged commit 2160b48 into indico:master Feb 20, 2024
@ThiefMaster ThiefMaster deleted the sui-dropdown branch February 20, 2024 15:42
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.

3 participants