Skip to content
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

#1976 Document Builder. Lists #2077

Merged
merged 14 commits into from
Dec 9, 2021
Merged

Conversation

BALEHOK
Copy link
Contributor

@BALEHOK BALEHOK commented Dec 7, 2021

Relates to #1976

  • Allow Row and Col as list elements
  • Fix unit tests
  • Code cleanup

Edit page:
image

Side panel:
image

@twschiller
Copy link
Contributor

twschiller commented Dec 8, 2021

Testing notes

We probably don't need to support this out of the gate, but I noticed manually providing a list errors out:

image

image

I wrote a jq brick with a hard-code list in the filter:

[
   {text: "Hola, mundo!"},
   {text: "Hello, world!"}
]

I'm getting some issue running the panel. I can help look into it with a debugger tomorrow

image

I also hit an issue where the builder would crash when I went to another extension in the devtools sidebar and then switched back to the new document I was working on. I need to find reproduction steps for that, though

@BALEHOK BALEHOK requested review from BLoe and twschiller December 8, 2021 16:41
@BALEHOK BALEHOK marked this pull request as ready for review December 8, 2021 16:42
@BALEHOK
Copy link
Contributor Author

BALEHOK commented Dec 8, 2021

Manually created array as a source for the list
image

If a list fails to render the rest of the panel is still there
image

<GridLoader />
) : (
<>
{rootDefinitions.map(({ documentElement, elementContext }, i) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Somehow I'm getting an Cannot read properties of undefined (reading 'map') error here

I think it happens when there's an error calculating the asyncState?

Copy link
Contributor

Choose a reason for hiding this comment

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

I updated this to display the error when the factory of useAsyncState fails

Copy link
Contributor

@twschiller twschiller left a comment

Choose a reason for hiding this comment

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

@BALEHOK Looking good. I will create a separate issue for running mapArgs in a different context

@twschiller
Copy link
Contributor

One improvement I'd make is to make element as the placeholder text for the list element field

@twschiller twschiller merged commit 30650f5 into main Dec 9, 2021
@twschiller twschiller deleted the feature/1976-doc-builder-lists branch December 9, 2021 19:03
@twschiller twschiller added this to the 1.4.11 milestone Dec 9, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants