[#8895] Refactor search profile, update alerts and add delete modal

[sevfurneaux]

Describe your changes
This PR refactors the search profile, to have a similar pattern of updating the alerts as the Kiezradars component where most of the state is in the parent.

It also updates the search profile alert text and adds the delete modal when deleting search profiles.

Tasks

• PR name contains story or task reference
• Steps to recreate and test the changes
• Documentation (docs and inline)
• Tests (including n+1 and django_assert_num_queries where applicable)
• Changelog

[hom3mad3]

i'd add a null check before calling showModal()

useEffect(() => {
  if (dialogRef.current) {
    dialogRef.current.showModal()
  }
}, [])

[sevfurneaux]

Thanks! Have added 👍

[hom3mad3]

we could probably save some performance and drop this useEffect, and add onKeyDown as dialog attribute/prop. see comment below

[sevfurneaux]

Nice, that's now removed.

[hom3mad3]

    <dialog 
      className="kiezradar__modal" 
      ref={dialogRef} 
      aria-modal="true" //can't remember whether we still need this
      aria-labelledby="modal-title"
      onKeyDown={(event) => {
        if (event.key === 'Escape') closeModal()
      }}
    >

[hom3mad3]

also, since we have a title we might add a aria-labelledby="modal-title" and the id="modal-title" to the h3 (adds more context than aria-modal alone)

[sevfurneaux]

Added that 👍

[hom3mad3]

what about const handleDelete = useCallback(async () => { ... and handleSubmit? to prevent unnecessary re-renders

[sevfurneaux]

Oh cool, yes! Both using useCallback now.

[hom3mad3]

what about destructuring to avoid redundancy ? const { id, name, query_text, districts, topics, project_types, status, organisations, plans_only, notification } = searchProfile;

[sevfurneaux]

Tried this, but the linter complains:

  const {
    query_text,
    districts,
    topics,
    project_types,
    status,
    organisations,
    plans_only
  } = searchProfile

  87:5  error  Identifier 'query_text' is not in camel case     camelcase
  90:5  error  Identifier 'project_types' is not in camel case  camelcase
  93:5  error  Identifier 'plans_only' is not in camel case     camelcase
  99:5  error  Identifier 'project_types' is not in camel case  camelcase
 106:6  error  Identifier 'query_text' is not in camel case     camelcase
 108:6  error  Identifier 'plans_only' is not in camel case     camelcase

Let's keep as is for now (and not alias to camelCase, as that is misleading what named properties are actually returned from a search profile).

Side note: I am always battling with this linter before pushing 🙈, mainly around formatting. Maybe in the future, Liquid can change to Prettier formatting, which formats on save in most editors.

[hom3mad3]

what do you think of adding <fieldset disabled={loading}> below <form> to prevent submitting multiple times?

[hom3mad3]

we probably should avoid directly handling the DOM here 🤔 💭

[hom3mad3]

sorry a string of meetings interrupted my review 🫠 i think i was going to suggest useRef here..

[sevfurneaux]

So it turns out by adding type="button to <button className="link" onClick={() => setIsEditing(false)}> means we no longer need onKeyDown as the browser now natively handles it 🎉

Unsure why doing this fixed it, as it is not the submit button, but prior to this, if we didn't have onKeyDown, Chrome was complaining:

"Form submission canceled because the form is not connected"

[hom3mad3]

Thanks ✨