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: partners highlight field on backend error #3448

Merged
merged 5 commits into from
May 17, 2023

Conversation

ludtkemorgan
Copy link
Collaborator

@ludtkemorgan ludtkemorgan commented May 12, 2023

Pull Request Template

Issue Overview

This PR addresses #3430, #3232

  • This change addresses the issue in full
  • This change addresses only certain aspects of the issue
  • This change is a dependency for another issue
  • This change has a dependency from another issue

Description

There are two issues happening on the Listing edit/add form in partners. Both are due to components re-rendering when they shouldn't need to.

1st issue: When clicking save the errors are not highlighting the fields that need resolution. This is happening because there is an additional re-render that is triggering the useEffects in Units.tsx and CommunityType.tsx, LeasingAgent.tsx. Both of which are resetting the values in the form,

  • Fix # 1: Change Reset in Units.tsx to a setValue when the value is undefined or null. One has to be selected so this works in all cases.
  • Fix Run micro-dev on port set by .env #2: Change reset in CommunityType.tsx to a setValue. This one works, but if an error happens on submission it will reset it back to the original value if you change it to the unset value
  • Fix Add Cypress to the public app #3: Change the clearErrors to only run if the value has changed (the user has typed or deleted something in the field)

2nd issue: When typing in most fields every character typed/deleted causes a re-render of all the form components. You can easily notice this if you have a photo already added and start typing characters in another field (e.g "Listing name"). The photo component can easily be spotted as being re-rendered

  • Fix: change the "onChange" commands to only clear the error if there is actually an error
  • My solution works a majority of the time, but it still sometimes causes additional re-renders

These are both things we should add or investigate further as the case in the 1st option. However, we should investigate what changed recently to cause all of these additional re-renders.

How Can This Be Tested/Reviewed?

Provide instructions so we can review.

Describe the tests that you ran to verify your changes. Please also list any relevant details for your test configuration.

Checklist:

  • My code follows the style guidelines of this project
  • I have added QA notes to the issue with applicable URLs
  • I have performed a self-review of my own code
  • I have reviewed the changes in a desktop view
  • I have reviewed the changes in a mobile view
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been merged and published in downstream modules
  • I have assigned reviewers
  • I have run yarn generate:client and/or created a migration if I made backend changes that require them
  • My commit message(s) is/are polished, and any breaking changes are indicated in the message and are well-described
  • Commits made across packages purposefully have the same commit message/version change, else are separated into different commits

Reviewer Notes:

Steps to review a PR:

  • Read and understand the issue, and ensure the author has added QA notes
  • Review the code itself from a style point of view
  • Pull the changes down locally and test that the acceptance criteria is met
  • Also review the acceptance criteria on the Netlify deploy preview (noting that these do not yet include any backend changes made in the PR)
  • Either explicitly ask a clarifying question, request changes, or approve the PR if there are small remaining changes but the PR is otherwise good to go

On Merge:

If you have one commit and message, squash. If you need each message to be applied, rebase and merge.

@netlify
Copy link

netlify bot commented May 12, 2023

Deploy Preview for bloom-exygy-dev ready!

Name Link
🔨 Latest commit 9d8361d
🔍 Latest deploy log https://app.netlify.com/sites/bloom-exygy-dev/deploys/6464e1ffa960f400088a6fe9
😎 Deploy Preview https://deploy-preview-3448--bloom-exygy-dev.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@ludtkemorgan ludtkemorgan added the 2 reviews needed Requires 2 more review before ready to merge label May 16, 2023
@ludtkemorgan ludtkemorgan requested review from emilyjablonski, ColinBuyck and YazeedLoonat and removed request for emilyjablonski May 16, 2023 14:22
@ludtkemorgan
Copy link
Collaborator Author

@ColinBuyck The bug you mentioned in standup #3232 was a similar issue so I fixed it I think with my most recent commit.

This is ready to review. As far as I know there is still one bug but it can be reviewed and QA'd for everything else.

Bug:

  1. Edit a listing that has a community type
  2. Change the following things:
  • Change the community type to a different type
  • Make a change in one of the other fields that will cause an error (e.g. delete the name field)
  1. Click save and the community type will be reset back to the original type

Copy link
Collaborator

@ColinBuyck ColinBuyck left a comment

Choose a reason for hiding this comment

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

Just a few nits 🪰
The error on the Jursidiction field doesn't seem to clear after selecting a jurisdiction
Screenshot 2023-05-16 at 3 24 29 PM

The leasing agent phone number doesn't highlight red despite being a required field
Screenshot 2023-05-16 at 3 26 34 PM

Copy link
Collaborator

@YazeedLoonat YazeedLoonat left a comment

Choose a reason for hiding this comment

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

once colin's comments are addressed I think this is good to go!

Copy link
Collaborator

@ColinBuyck ColinBuyck left a comment

Choose a reason for hiding this comment

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

Gave it another pass and this LGTM once the two pieces above are addressed 🧊

@ColinBuyck ColinBuyck removed the 2 reviews needed Requires 2 more review before ready to merge label May 17, 2023
Copy link

@accesslint accesslint bot left a comment

Choose a reason for hiding this comment

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

There are accessibility issues in these changes.

@ludtkemorgan
Copy link
Collaborator Author

@YazeedLoonat @ColinBuyck Thanks for reviewing! I fixed the leasing agent phone number and jurisdiction issues identified.

@ludtkemorgan ludtkemorgan merged commit f22adfb into main May 17, 2023
ludtkemorgan added a commit to housingbayarea/bloom that referenced this pull request May 17, 2023
* fix: partners highlight field on backend error

* fix: community type and disableUnitsAccordion fix

* fix: unit type fix for partial units

* fix: review comment addressed

* fix: phone number fix
ludtkemorgan added a commit to housingbayarea/bloom that referenced this pull request May 18, 2023
* fix: partners highlight field on backend error

* fix: community type and disableUnitsAccordion fix

* fix: unit type fix for partial units

* fix: review comment addressed

* fix: phone number fix
ludtkemorgan added a commit to housingbayarea/bloom that referenced this pull request May 18, 2023
* fix: uptake latest uic Modal, ActionBlock, FormCard breaking changes (bloom-housing#3358)

* feat: upgrade react to 18 (bloom-housing#3360)

* feat: upgrade nextjs to 13 (bloom-housing#3375)

* feat: upgrade nextjs to 13

* fix: attempt to get cypress test working

* feat: changing auth over to cookies (bloom-housing#3357)

* fix: resolves issues around markedAsDuplicate (bloom-housing#3373)

* fix: react type errors (bloom-housing#3382)

* refactor: add cloudinary fxn to partners (bloom-housing#3393)

* refactor: uptake seeds FormErrorMessage (bloom-housing#3369)

* fix: add startDate to open house submit event (bloom-housing#3399)

* fix: add three new fields to base view (bloom-housing#3406)

* feat: removing sensative info from leasing agent (bloom-housing#3409)

* feat: removing sensative info from leasing agent

* fix: adding swagger changes

* fix: updates for tests

* chore(deps): bump cookiejar from 2.1.2 to 2.1.4 (bloom-housing#3295)

Bumps [cookiejar](https://github.com/bmeck/node-cookiejar) from 2.1.2 to 2.1.4.
- [Release notes](https://github.com/bmeck/node-cookiejar/releases)
- [Commits](https://github.com/bmeck/node-cookiejar/commits)

---
updated-dependencies:
- dependency-name: cookiejar
  dependency-type: indirect
...

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* fix: updates around cookies (bloom-housing#3405)

* fix: updates around cookies

* fix: creating new migration for token -> code

* fix: Searching on the applications table causes the page to crash (bloom-housing#3408)

* fix: pass proper value to to_tsquery function

* fix: search applications using ILIKE

* fix: change where to orWhere

* feat: add application search by confirmation code

* updates proxy to support access control allow list (bloom-housing#3407)

* feat: updates proxy to support access control allow list

* fix: remove downstream access-control-allow-origin

* fix: update readme for m1

* fix: move purge call to the backend

* fix: test fix and add await

* fix: moving cache purge to helper

---------

Co-authored-by: Morgan Ludtke <[email protected]>
Co-authored-by: Yazeed Loonat <[email protected]>

* fix: escape quote in translation update

* fix: add translation for 64 characters error (bloom-housing#3423)

* fix: downgrade version of nest axios (bloom-housing#3419)

* fix: now removes criteria file if a url is input (bloom-housing#3421)

* fix: remove check in test not applicable for hba

* fix: update ui-c to latest version (bloom-housing#3420)

* fix: update application test

* feat: 3291/listing export take 2 (bloom-housing#3424)

* fix: functional frontend

* fix: hooks and service updates

* fix: hitting service file

* fix: wip config work

* fix: wip config 2

* fix: completed query updates

* fix: complete column naming and basic formatting

* fix: clean up formatting

* fix: wip testing debugging

* fix: functional unit tests

* fix: cypress tests + formatting

* fix: unadded test changes

* fix: internal csv testing

* fix: exporter test fix

* fix: more detailed csv checks

* fix: testing + formatting tweaks

* fix: exporter testing improvements

* fix: updates per pr feedback

* fix: match config pattern

* fix: add close status option

* fix: reset netlify testing

* fix: final cleanup

* fix: rent type formatting

* fix: remove console log

* fix: missing state data (bloom-housing#3450)

* feat: adding knownError flag

* feat: adding knownError flag

* fix: partners highlight field on backend error (bloom-housing#3448)

* fix: partners highlight field on backend error

* fix: community type and disableUnitsAccordion fix

* fix: unit type fix for partial units

* fix: review comment addressed

* fix: phone number fix

---------

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: Yazeed Loonat <[email protected]>
Co-authored-by: Emily Jablonski <[email protected]>
Co-authored-by: Krzysztof Zięcina <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: Sean Albert <[email protected]>
Co-authored-by: ColinBuyck <[email protected]>
chriscasto added a commit to metrotranscom/doorway that referenced this pull request May 31, 2023
* feat: 3291/listing export take 2 (bloom-housing#3424)

* fix: functional frontend

* fix: hooks and service updates

* fix: hitting service file

* fix: wip config work

* fix: wip config 2

* fix: completed query updates

* fix: complete column naming and basic formatting

* fix: clean up formatting

* fix: wip testing debugging

* fix: functional unit tests

* fix: cypress tests + formatting

* fix: unadded test changes

* fix: internal csv testing

* fix: exporter test fix

* fix: more detailed csv checks

* fix: testing + formatting tweaks

* fix: exporter testing improvements

* fix: updates per pr feedback

* fix: match config pattern

* fix: add close status option

* fix: reset netlify testing

* fix: final cleanup

* fix: rent type formatting

* fix: remove console log

* fix: missing state data (bloom-housing#3450)

* feat: adding knownError flag

* feat: adding knownError flag

* fix: partners highlight field on backend error (bloom-housing#3448)

* fix: partners highlight field on backend error

* fix: community type and disableUnitsAccordion fix

* fix: unit type fix for partial units

* fix: review comment addressed

* fix: phone number fix

* fix: stop transforming null to zero (bloom-housing#3453)

* fix: stop transforming null to zero

* fix: other fields defaulting to zero

* fix: public multiple photos from detroit (bloom-housing#3390)

* feat: add multiple photos

* feat: add tests

* feat: add images to seeds

* fix: return array when no image

* fix: shared helpers photos tests

* fix: add class with variables to parent element

* fix: make actions inside content section optional on desktop

* fix: pass modalCloseInContent prop to listing view's ImageCard

* fix: remove redundant image-card class

* chore: upgrade ui-c

* fix: remove redundant order column

* fix: change font for drawer title

* fix: remove default images value when editing

---------

Co-authored-by: ColinBuyck <[email protected]>
Co-authored-by: Yazeed Loonat <[email protected]>
Co-authored-by: Krzysztof Zięcina <[email protected]>
Co-authored-by: Chris Casto <[email protected]>
@ColinBuyck ColinBuyck mentioned this pull request Jun 1, 2023
19 tasks
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