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: enable utilities and accessibility features #4503

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

ludtkemorgan
Copy link
Collaborator

This PR addresses #4440 #4439

  • Addresses the issue in full
  • Addresses only certain aspects of the issue

Description

Listing utilities and accessibility features functionality already exist. However it was not working correctly. This PR fixes all of the bugs as well as connecting it to the new feature flag functionality

NOTE: the jurisdiction table fields have also been removed since we can now use the feature flag table

How Can This Be Tested/Reviewed?

Both feature flags are added to the staging seeded data for "Bloomington" jurisdiction so the following can be tested with a new reseed

  • Create a new listing in partners for the "Bloomington" jurisdiction.
    • Verify that the Utilities checkbox section and the accessibility features checkbox section appear
    • Verify that you can select specific utilities and accessibility features. On save make sure they are saved correctly
  • Edit the listing and change the utilities/accessibility features
    • Verify that the fields are correctly updated on save
  • Create a new listing not for "Bloomington" jurisdiction and verify that the two sections don't appear
  • Go to the public site and verify that the first listing created shows the correct utilities and accessibility features on the listing details page

Author Checklist:

  • Added QA notes to the issue with applicable URLs
  • Reviewed in a desktop view
  • Reviewed in a mobile view
  • Reviewed considering accessibility
  • Added tests covering the changes
  • Made corresponding changes to the documentation
  • Ran yarn generate:client and/or created a migration when required

Review Process:

  • Read and understand the issue
  • 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
  • Either (1) explicitly ask a clarifying question, (2) request changes, or (3) approve the PR, even if there are very small remaining changes, if you don't need to re-review after the updates

Copy link

netlify bot commented Dec 5, 2024

Deploy Preview for partners-bloom-dev ready!

Name Link
🔨 Latest commit 28bc43b
🔍 Latest deploy log https://app.netlify.com/sites/partners-bloom-dev/deploys/6789407bcc0d240008aa03f8
😎 Deploy Preview https://deploy-preview-4503--partners-bloom-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 configuration.

Copy link

netlify bot commented Dec 5, 2024

Deploy Preview for bloom-exygy-dev ready!

Name Link
🔨 Latest commit 28bc43b
🔍 Latest deploy log https://app.netlify.com/sites/bloom-exygy-dev/deploys/6789407b5a6bc10008e8bd15
😎 Deploy Preview https://deploy-preview-4503--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 configuration.

@ludtkemorgan ludtkemorgan marked this pull request as ready for review December 5, 2024 17:37
@ludtkemorgan ludtkemorgan added the 2 reviews needed Requires 2 more review before ready to merge label Dec 19, 2024
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.

looks good, one small 🧹

@YazeedLoonat YazeedLoonat added 1 review needed Requires 1 more review before ready to merge and removed 2 reviews needed Requires 2 more review before ready to merge labels Jan 3, 2025
@ludtkemorgan ludtkemorgan force-pushed the 4440/accessibility branch 2 times, most recently from c72d63a to de344fa Compare January 3, 2025 19:34
Copy link
Collaborator

@mcgarrye mcgarrye left a comment

Choose a reason for hiding this comment

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

issue: signed in as an admin, I can still see the sections for a listing for another jurisdiction on creation

Screen.Recording.2025-01-07.at.3.22.30.PM.mov

@ludtkemorgan
Copy link
Collaborator Author

@mcgarrye good catch. I was missing an s and it now works correctly so is ready for re-review

@emilyjablonski
Copy link
Collaborator

emilyjablonski commented Jan 7, 2025

This feels like a very niche use case, but if I take a listing that has both utilities and a11y features, and then change the jurisdiction to be one that doesn't include them, the listing keeps the data. Otherwise looks great!

Screenshot 2025-01-07 at 4 43 27 PM
I also think the spacing above the utilities here is too big, and the text size is small enough to be considered inaccessible in the bulleted list - could we increase it a bit? Is there a design for what this should look like in core?

@ludtkemorgan
Copy link
Collaborator Author

@emilyjablonski There is no design for this as these fields already existed in both public and partners, there was just some bugs with the saving of the values. So the design is the same as when it was implemented 3 years ago. Looks like that styling is coming from a ui-components component AdditionalFees.

@emilyjablonski
Copy link
Collaborator

@ludtkemorgan I'm about to rewrite that section, so it's no worries to me!

Copy link
Collaborator

@emilyjablonski emilyjablonski left a comment

Choose a reason for hiding this comment

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

🐧

@emilyjablonski emilyjablonski added ready to merge Should be applied when a PR has been reviewed and approved and removed 1 review needed Requires 1 more review before ready to merge labels Jan 15, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready to merge Should be applied when a PR has been reviewed and approved
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants