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

Post Listing Card #34

Merged
merged 26 commits into from
Jun 17, 2024
Merged

Post Listing Card #34

merged 26 commits into from
Jun 17, 2024

Conversation

adriencyberspace
Copy link
Collaborator

@adriencyberspace adriencyberspace commented Jun 12, 2024

Pull Request Template

Issue Overview

This PR addresses #98

  • 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

This PR edits the SearchResults listing card component and surrounding components to accomodate new designs, by:

  • removing unneeded components and code
  • separating out reusable functions into utils files
  • editing the component and scss where needed to fit designs
  • saving code that will be used in Phase 2 in separate SearchResultsWithTexting file

NOTE: subcategories will be in separate PR

How Can This Be Tested/Reviewed?

Visit any search results page (i.e. click any of the links on homepage like Food, Shelters, etc. or visiting /search) and comparing new designs to mockups. Use tab navigation and check on all screen sizes.

Make sure you have askdarcel-api running locally. LMK if you haven't set that up and I can walk you through it.

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
  • I have assigned reviewers
  • My commit message(s) is/are polished, and any breaking changes are indicated in the message and are well-described

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.

@rosschapman
Copy link
Collaborator

@adriencyberspace Nice! Happy to leave ✅ to unblock merge since I know it's crunch time. I haven't done much CSS review recently so I'm still figuring out the best way to evaluate it for you. One thing that stood out is a lot of "magic number" values. I'm curious why some values are stored in variables and some not.

I'll ping you next week about getting my api running to help with visual QA if necessary. I'm missing environment configs and don't have access to ST's notion page (yet).

app/utils/numbers.ts Outdated Show resolved Hide resolved
* Renders address metadata based on the `addresses` array in the `hit_` object.
*
*/
export const renderAddressMetadata = (hit_: SearchHit): JSX.Element => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

🤔 I'm curious why the parameter has an underscore. Is this a naming convention we use for some particular case?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think it's a parameter they were using but this helper function came with the project. I just moved it into this file to declutter the component it was in.

*
*/
export const renderAddressMetadata = (hit_: SearchHit): JSX.Element => {
if (!hit_.addresses || hit_.addresses.length === 0) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggestion: What about doing an Array.isArray(hit_.addresses) here? This hardens a bit more against a potentially malformed response from Algolia since it will do an existence check and runtime type check. For example, this nonsense would pass this check:

hit_ = {addresses: '[Address, Address]'}

This assumes there's no other sanitization of the response farther up in the code. Maybe you're thinking about it already for Phase 2+ but I always love doing these kind of data processing checks in the component javascript so by the time we need to do any presentational rendering in the return statement we already know the value.

I know we have relatively tight control over the api, I'm just coming off an experience working with a lot of third-party APIs that were hard to trust. So I wonder about this type of thing 💭💭💭🤩.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Same as above! This came with the project so I think we should leave it for now given the tight timeline and hopefully have a longer phase where we can optimize.

Co-authored-by: Ross Chapman <[email protected]>
@adriencyberspace adriencyberspace merged commit da1a976 into development Jun 17, 2024
2 checks passed
@adriencyberspace adriencyberspace deleted the listing-card branch June 17, 2024 16:22
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.

2 participants