Remove asyncpaginate in widgets

giuliaghisini commented 2 years ago

To avoid problems in widgets that show vocabularies like:

vocaboulary has more than 25 items, so if you had selected a 28th item the select was empty when remounting component because item was not loaded in first 25 items
vocabulary widgets in babel view were empty if before you had open the form of the source item
and other problems when editing and switching form tabs

we decided to remove pagination for vocabulary widgets and introduce subrequests to load the correct vocabulary in correct language.

ISSUE: https://github.com/plone/volto/issues/2782

giuliaghisini commented 2 years ago

@giuliaghisini please, check the tests and remove .env.local file. Once done this, LGTM

done!

giuliaghisini commented 2 years ago

@sneridagh @tisto i'm not able to test and fix failing tests because ci:start-api-plone command fails:

Version and requirements information containing plone.schema: [versions] constraint on plone.schema: 1.2.0 Requirement of plone.restapi==8.13.0: plone.schema>=1.2.1 While: Installing instance. Error: The requirement ('plone.schema>=1.2.1') is not allowed by your [versions] constraint (1.2.0)

tisto commented 2 years ago

@giuliaghisini can you pls try to merge master into your branch. We had an issue recently, maybe you branched in a bad moment.

giuliaghisini commented 2 years ago

@giuliaghisini can you pls try to merge master into your branch. We had an issue recently, maybe you branched in a bad moment.

i tried now, but still having same problems..

tisto commented 2 years ago

@giuliaghisini locally? Then try to do a "make clean" and re-build.

sneridagh commented 2 years ago

@giuliaghisini works here, even with the latest plone docker image. Can you try again?

sneridagh commented 2 years ago

@giuliaghisini also, regarding this PR. We've hit a culprit with one of our vocabularies, that include more than 6k entries. I does this change try to mount a component with 6k options? If so, I think we need to rethink it, if the options count is big, then make the select act like a "search only" field, not showing all of them. Do you think it's possible? I didn't have time to look at the react-select implementation or recommendations. Maybe there's a virtualisation option?

giuliaghisini commented 2 years ago

@giuliaghisini also, regarding this PR. We've hit a culprit with one of our vocabularies, that include more than 6k entries. I does this change try to mount a component with 6k options? If so, I think we need to rethink it, if the options count is big, then make the select act like a "search only" field, not showing all of them. Do you think it's possible? I didn't have time to look at the react-select implementation or recommendations. Maybe there's a virtualisation option?

6k entries °_° .. i've to take a look and thinking about it... but maybe the best thing is to use a different widget for this situations and we have to implement it

sneridagh commented 2 years ago

@giuliaghisini yeah, I know... So, I've found:

https://codesandbox.io/s/lxv7omv65l?file=/index.js https://github.com/bvaughn/react-window

looks promising... being the fact that the select is already lazy loaded, I don't think that's any problem with adding this...

Also FTR, we should also add this other "helper" to the selects: https://react-select.com/advanced#sortable-multiselect https://www.npmjs.com/package/react-sortable-hoc

What do you think?

sneridagh commented 2 years ago

@tiberiuichim historically, this one had, because of select internals, and to adapt info coming from the value/values and the store... Do you think there's a way to improve that?

tiberiuichim commented 2 years ago

@sneridagh https://github.com/plone/volto/blob/49405e949df2922d6ac936d221717c2fa7681ad2/src/components/manage/Widgets/SelectWidget.jsx#L238-L248

I understand that the API of react-select is probably silly and we probably need to call that normalizeValue, but it seems like all we're doing is keeping a copy of the select value in the state, when the select value should always be derivable from the input value.

tiberiuichim commented 2 years ago

In any case, it's not something we have to fix now.

sneridagh commented 2 years ago

@tiberiuichim I remember that it was not that straightforward (what I don't remember is why), but we can try to improve it.

sneridagh commented 2 years ago

@tiberiuichim anyways I think it's worth enough exploring, if we want to keep the whole story sane. Also: at some point we need to fusion all the related widgets into one.

sneridagh commented 2 years ago

nzambello commented 2 years ago

@tiberiuichim I remember that it was not that straightforward (what I don't remember is why), but we can try to improve it.

I also remember something about this: react-select has something that was somehow persistent and it wasn't updating correctly unless verbosely setting a new value from an internal state (otherwise it uses its internal state). But I don't remember anything more specific and I won't swear about it 😂

BTW yes, we should investigate and improve it if possible. Maybe in another PR (while opening an issue for that if not fixable now).

sneridagh commented 2 years ago

I also remember something about this: react-select has something that was somehow persistent and it wasn't updating correctly unless verbosely setting a new value from an internal state (otherwise it uses its internal state). But I don't remember anything more specific and I won't swear about it 😂

Yeah now that you mention it... I think it was something weird like it.

tiberiuichim commented 2 years ago

https://github.com/plone/volto/issues/2810#issuecomment-961774250

But yeah, probably we can't really recreate the component all the time, as it would break dropdowns and whatever. Too bad.

sneridagh commented 2 years ago

So, I still have a concern, about really big vocabularies (eg. a members, users vocab), that we have in our current project. There is an option to load react-select/async in a nice way, without preloading all the vocab. Like:

      <AsyncSelect
        cacheOptions
        defaultOptions={[]}
        loadOptions={promiseOptions}
        noOptionsMessage={()=>"Please start typing to select an element"}
      />

Question is: Should we go for an specialised widget, or should we integrate it in the existing ones? Passing an isAsync prop is an option, even from Python, I have a PoC working:

    directives.widget(
        "members",
        frontendOptions={"widget": "select", "widgetProps": {"isAsync": True}},
    )

What do you think?

tiberiuichim commented 2 years ago

@sneridagh can we go with async vocab always? Then we don't need extra PRs in python.

sneridagh commented 2 years ago

@tiberiuichim no extra PRs needed, that already works well, everything under frontendOptions are passed.

Always async is not an option, since you want to "see" the options 90% of the times, mostly because you don't know them upfront. In async, it doesn't search until you type.

sneridagh commented 2 years ago

@giuliaghisini @pnicolli I've found some culprits around this subject.

I would like to discuss with all of you about them, if you have time at some point next week it would be great.

I'll try to explain briefly:

The use cases and combinations around the selects are huge

With the current state of the PR, we are forcing all the Arrays to be Createable which is wrong, and could lead to backend "Constraint not satisfied" for all vocabs (or fixed list of Choices) that forces to grab one of them.

Then, the combinations are vast, and of course, we need to cover them all, and we all know that Plone and the p.restapi serialisation of those are simply not sufficient.

So, we need an army of tests, defining all the combinations and test them properly. And, eventually, storybook powered stories with all of them. I suggest using https://github.com/collective/example.contenttype and a custom Cypress fixture (or add it to the core-sandbox one), then start testing against it.

Not batching is fine, but we should revisit some basics

I was implementing a "just autocomplete" widget, so no search is triggered on mount, until the user types. This works well for big vocabularies. However, again, the api is unable of detecting them upfront, and tell the UI to use this. Also, realised that we were doing the calls wrong all this time, because react-select works with the loadOptions prop in an async way, so use the values in the store might be stale, making the use of the store useless.

We need to implement such a use case.

Regarding not batching, it's ready on p.restapi, we can change the calls to use it.

Reordering wrapping an arrayed (tokenised) Async `react-select` do not work

T_T

Vocabulary action

Yesterday I changed the shape of how we retrieve the vocabulary name from the URL, so that will need refactor. Also, I would like to change it to stop using positional arguments there (in fact, I would like to remove them from everywhere it makes sense, so more that two arguments => object-like arguments).

What now in the short term?

So, the paginated approach works in a quirky way. Could we rethink the approach of the loadOptions and give it another chance to work? I used this in my autocomplete widget:

  loadOptions = (search) => {
    // Implement a debounce of 400ms and a min search of 3 chars
    if (search.length > 2) {
      if (this.timeoutRef.current) clearTimeout(this.timeoutRef.current);
      return new Promise((resolve) => {
        this.timeoutRef.current = setTimeout(async () => {
          resolve(
            await this.props
              .getVocabulary(
                this.props.vocabBaseUrl,
                search,
                undefined,
                -1,
                this.props.intl.locale,
              )
              .then((resp) => {
                return resp.items.map((item) => ({
                  label: item.title,
                  value: item.token,
                }));
              }),
          );
        }, 400);
      });
    } else {
      return [];
    }
  };

So I just pass it the values right away from the call to the action, no store involved.

If after giving another chance, does not work, then we abandon it definitely. However, I'm still a bit weary of asking for all the vocabs always...

@plone/volto-team Anyways, I would like to discuss this more with all of you "live".

Mid term

We go all in with the new widget testing approach and unify the selects into one smartypants widget.

sneridagh commented 2 years ago

Summary of the meeting:

[x] Try to complete and improve this PR whenever necessary
[x] Refactor signature of getVocabularies action
[x] Create-ables only whenever required
[ ] Remove the "no value" since we have now isClearable?
[x] Add the ability to pass props from the Python side
[x] Add the AutoCompleteWidget (for use in large vocabularies)
[ ] Add documentation
[x] Add as much Cypress tests as possible

Mid term:

Uber-widget
Change the decider to support this
Add example.contenttype-like testing approach
Army of Cypress tests

sneridagh commented 2 years ago

@plone/volto-team and it's green...

If anyone could help trying it out, it would be great! We need feedback of it.

What's left?

Documentation
What do we do with "No value" forced option

TL;DR, we are adding it, since there are some situations, that we need to "unset", specially on the boolean selects (eg. the discussion metadata field) so nor True or False option. But react-select has the isClearable that I added to all selects now. Which is better, IMO. What should we do? We can be conservative, and leave it, then remove it at a later point.

I would like to merge it ASAP, so 14 would be almost ready.

giuliaghisini commented 2 years ago

i'd like to test it in next days, maybe monday or tuesday!

On Fri, Dec 10, 2021 at 9:48 AM Tiberiu Ichim @.***> wrote:

@.**** approved this pull request.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/plone/volto/pull/2805#pullrequestreview-828605109, or unsubscribe https://github.com/notifications/unsubscribe-auth/AMMBWALFUBT3GQRSMK6LYOLUQG5GZANCNFSM5HF6IKKA . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.

giuliaghisini commented 2 years ago

@plone/volto-team and it's green...

If anyone could help trying it out, it would be great! We need feedback of it.

What's left?

Documentation

What do we do with "No value" forced option

TL;DR, we are adding it, since there are some situations, that we need to "unset", specially on the boolean selects (eg. the discussion metadata field) so nor True or False option. But react-select has the isClearable that I added to all selects now. Which is better, IMO. What should we do? We can be conservative, and leave it, then remove it at a later point.

I would like to merge it ASAP, so 14 would be almost ready.

i'd like to test it maybe on monday or tuesday, but if you need it asap for me is fine

sneridagh commented 2 years ago

I think that's it :) I've already tested in our last project, it works for me. Also the new autocomplete widget as well.

I would merge it right away, although we could use some more testing.

@giuliaghisini let's wait for you and your tests, and others if possible.

But let's put a milestone in mid next week to make the final release.

giuliaghisini commented 2 years ago

@sneridagh great work! I've done several tests and for me it has green light!

plone / volto