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

Fix form builder and default forms #870

Merged
merged 5 commits into from
Jan 14, 2025
Merged

Fix form builder and default forms #870

merged 5 commits into from
Jan 14, 2025

Conversation

kalilsn
Copy link
Member

@kalilsn kalilsn commented Dec 22, 2024

Issue(s) Resolved

Resolves #868

High-level Explanation of PR

There are 3 different bugs causing unwanted fields to show up on the pubs page. This fixes two of them:

  • Recently we removed what looked like an extraneous aliasing of id to elementId. This made it impossible to delete form elements because we use the presence of elementId to check whether the element has been saved already. If it hasn't been saved, it's immediately removed from the form. This change caused saved elements to have the same behavior, rather than marking them with { deleted: true } and sending them to the backend to actually get removed.
  • In the default form for pub updates we render an input for any value that exists on the pub, even if it's not on the form. But we were also inadvertently rendering an element for every field on the pub type.

The third bug is just that we often create "empty" values and we don't have any way to delete them. So even after merging this and actually removing an element from the form, it may still show up on the pub update form until we make it possible to delete values. I would just filter these out in the query, except that there are a variety of empty values ("", " ",[{}]) not just null so I don't really want to make a decision about how to handle that now. The create pub form should update correctly though!

Test Plan

Same as the issue!

Screenshots (if applicable)

Notes

@@ -35,8 +35,7 @@ type GetPubFieldsInput =
* Get pub fields
*
* @param props - When nothing is supplied, return all the pub fields
* @param props.pubId - When supplied, return all the pub fields associated with the pub through pub values and the pub type of that pub
* When props.valuesOnly is true, only return the pub fields associated with the pub through pub values, not through the pub type
Copy link
Member Author

@kalilsn kalilsn Dec 22, 2024

Choose a reason for hiding this comment

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

we stopped using the valuesOnly prop at some point, but the query was never updated. and now valuesOnly is actually the behavior we always want so i've updated the comments and removed the additional pub type fields join from the query.

i also checked and the only place we use this version of the query (passing the pubid) is on the pub editor that i'm fixing here.

return rest;
const { slug, id, ...rest } = e;
// Rename id to avoid conflict with rhf generated id
return { ...rest, elementId: id };
Copy link
Member Author

Choose a reason for hiding this comment

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

rather than restore the alias in the query, i rename id here, which is a little closer to where we actually use the elementId prop

@kalilsn kalilsn changed the title Don't render unnecessary fields on pub edit page Fix form builder and default forms Dec 23, 2024
@kalilsn kalilsn force-pushed the kalilsn/fix-element-deletion branch from abe47ae to e091773 Compare December 23, 2024 17:25
Copy link
Contributor

@allisonking allisonking left a comment

Choose a reason for hiding this comment

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

tested this out and was able to delete + update the default form and have it reflect in the update page!

@tefkah
Copy link
Contributor

tefkah commented Jan 14, 2025

Just for my understanding, does this now still let the pub/edit page render fields on the pub that are not on the default form?

bc i think it should! otherwise how are you ever to change values whose fields are not on the default form?

edit: tested it and yes it still does!

@3mcd 3mcd merged commit 87cec2b into main Jan 14, 2025
8 checks passed
@3mcd 3mcd deleted the kalilsn/fix-element-deletion branch January 14, 2025 15:05
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.

Default form does not load as default form
4 participants