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(homepage): placeholder inputs #1451

Merged
merged 16 commits into from
Sep 6, 2023
Merged

fix(homepage): placeholder inputs #1451

merged 16 commits into from
Sep 6, 2023

Conversation

seaerchin
Copy link
Contributor

@seaerchin seaerchin commented Aug 24, 2023

Problem

Previously, we always prefilled sections on initial add - this allows people to slip up and accidentally click save even when it was the default values.

Closes IS-500

Solution

  • Separate prefill values and placeholder values. This then allows us to add an empty block (on left panel) while still displaying a sane preview value on the preview panel. This was done through first filtering by which properties have an empty value then merging with default values on render.
  • fix existing validation - we always check if length < 0, which will never be true.
  • extract stuff out from current validation to reuse - this was because the validation also sets some additional properties on an error object.
  • write util function to compute hasError from the formState - this is required because errors are only set on change. this means that, for example, users can add an empty block and save instantly (as no change was made), which runs counter to what we want.
  • add onBlur = onChange. as the value is controlled by us and there's no value change on blur, this only serves to retrigger validation on blur events so that we inform the user about what failed validation (empty input on initial create for eg)

Tests

  • trigger validation on existing items
    • this can be done through exceeding max limit or deleting stuff on required fields
  • create a new block
  • save should be disabled
  • expand accordion
  • click on input
  • click away
  • error message should pop up
  • save should still be disabled

Notes

Feels like we're reinventing the wheel here already. i don't want to put more effort into this but our validation strategy feels abit off as closing the accordion, for example, does not trigger validation.

maybe can consider shifting to a form lib that does this for us but is a significant effort so i think we should just live with this as an intermediate measure until we decide that we want to nudge website builders in the right dir

@sehyunidaaa - maybe we can prompt the isOptional already otherwise feels like users might grapple with this

@seaerchin seaerchin requested a review from a team August 24, 2023 12:10
@seaerchin seaerchin changed the title Fix/placeholder inputs fix(homepage): placeholder inputs Aug 24, 2023
@seaerchin seaerchin temporarily deployed to staging August 24, 2023 12:16 — with GitHub Actions Inactive
@seaerchin seaerchin temporarily deployed to staging August 24, 2023 12:28 — with GitHub Actions Inactive
src/layouts/EditHomepage/EditHomepage.jsx Outdated Show resolved Hide resolved
Comment on lines 44 to 45
<div className={editorStyles["bp-dropdown-item"]}>
<h5>{title}</h5>
<h5>{title || DROPDOWN_ELEMENT_SECTION.title}</h5>
</div>
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a reason why we pass in hero section this way instead of like the other preview sections? (destructuring existing object so that only non-blank params will overwrite)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

don't the other preview adopt this pattern also?

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 cos this way was less work - i'm open to keeping it consistent tbh

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is abit annoying to do here cos we have to do a type cast + overwrite a child array. lmk if this is a blocker for u

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm okay with leaving it as is if it's annoying to implement!

src/utils/validators.js Outdated Show resolved Hide resolved
@seaerchin seaerchin temporarily deployed to staging August 29, 2023 04:07 — with GitHub Actions Inactive
@seaerchin seaerchin temporarily deployed to staging August 29, 2023 04:09 — with GitHub Actions Inactive
@seaerchin seaerchin requested review from alexanderleegs and a team August 29, 2023 04:13
@seaerchin seaerchin temporarily deployed to staging August 29, 2023 04:22 — with GitHub Actions Inactive
Base automatically changed from chore/tidy-edithomepage to develop August 29, 2023 05:50
@seaerchin seaerchin force-pushed the fix/placeholder-inputs branch from d868e2b to 0a7abe7 Compare August 29, 2023 05:51
@seaerchin seaerchin temporarily deployed to staging August 29, 2023 05:51 — with GitHub Actions Inactive
@seaerchin seaerchin temporarily deployed to staging August 29, 2023 06:04 — with GitHub Actions Inactive
src/templates/homepage/InfopicLeftSection.jsx Outdated Show resolved Hide resolved
src/layouts/EditHomepage/utils.ts Outdated Show resolved Hide resolved
src/layouts/EditHomepage/utils.ts Outdated Show resolved Hide resolved
src/layouts/EditHomepage/utils.ts Show resolved Hide resolved
src/layouts/EditHomepage/utils.ts Show resolved Hide resolved
src/layouts/EditHomepage/utils.ts Show resolved Hide resolved
@seaerchin seaerchin temporarily deployed to staging August 30, 2023 08:20 — with GitHub Actions Inactive
@seaerchin seaerchin requested review from QiluXie and a team August 30, 2023 08:20
@seaerchin seaerchin temporarily deployed to staging August 30, 2023 08:32 — with GitHub Actions Inactive
Copy link
Contributor

@QiluXie QiluXie left a comment

Choose a reason for hiding this comment

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

lgtm, thanks @seaerchin for addressing the comments quickly

@seaerchin seaerchin temporarily deployed to staging September 6, 2023 08:39 — with GitHub Actions Inactive
@seaerchin seaerchin force-pushed the fix/placeholder-inputs branch from 6792d9d to 84a83e6 Compare September 6, 2023 08:43
@seaerchin seaerchin merged commit a728258 into develop Sep 6, 2023
4 checks passed
@seaerchin seaerchin deleted the fix/placeholder-inputs branch September 6, 2023 08:47
@seaerchin seaerchin temporarily deployed to staging September 6, 2023 08:56 — with GitHub Actions Inactive
@alexanderleegs alexanderleegs restored the fix/placeholder-inputs branch September 7, 2023 04:28
@alexanderleegs alexanderleegs temporarily deployed to staging September 7, 2023 04:42 — with GitHub Actions Inactive
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