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: under construction #1607

Merged
merged 5 commits into from
Mar 30, 2023
Merged

fix: under construction #1607

merged 5 commits into from
Mar 30, 2023

Conversation

ColinBuyck
Copy link
Collaborator

@ColinBuyck ColinBuyck commented Mar 28, 2023

Pull Request Template

Issue Overview

This PR addresses #1564

  • 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 changes all visual references of "Coming Soon" to "Under Construction" without making any backend changed. This includes image tag labeling, homepage "under construction section", filtering, what to Expect text, partner's form questions and details, and a new display pattern to highlight when residents should apply.

Note (cc @eajensenwa):
I made the decision to still include the "Residents should apply in..." messaging when there was only a year present but no season since I felt it was still applicable for properties that wouldn't be open for a while and didn't have a season yet set. Let me know if this is what you'd expect.

Also, made two small assumptions on the "Residents should apply in" Figma. One that the font weight of 400 in the design differing from the Alert Box default of 500 is not worth pursuing customization. Secondly, the icon used in the Figma isn't one of our internal icons (hollow info icon is but solid is not) and with how we type the Alert Box component we would have to make a change to the component in UI-C. I assumed this change was also not worth the additional time. Let me know if you agree with these assumptions!

Note (cc @sarahlazarich ): There aren't currently any what to expect translations so I did not implement the "This property is still under construction" translations

How Can This Be Tested/Reviewed?

One way to test this would be to pull this down locally, reseed!, and create a new listing.

  1. Set it as Under Construction (ensure text update there)
  2. Save
  3. Verify the updates are reflected on the homepage (under construction section, see all under construction, image tag)
  4. Check translations
  5. Go back to partners and add marketing season and date to same listing.
  6. Save
  7. View listing on detail view
  8. Verify new "Residents should apply in..." messaging matches Figma
  9. Check translations
  10. Change window sizing to ensure mobile view matches Figma

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
  • I have exported any new pieces added to ui-components
  • 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 Mar 28, 2023

Deploy Preview for detroit-storybook-dev ready!

Name Link
🔨 Latest commit 7e3fbe6
🔍 Latest deploy log https://app.netlify.com/sites/detroit-storybook-dev/deploys/6423880fee10740008777c0f
😎 Deploy Preview https://deploy-preview-1607--detroit-storybook-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.

@netlify
Copy link

netlify bot commented Mar 28, 2023

Deploy Preview for detroit-public-dev ready!

Name Link
🔨 Latest commit 7e3fbe6
🔍 Latest deploy log https://app.netlify.com/sites/detroit-public-dev/deploys/6423880f58a929000805cc81
😎 Deploy Preview https://deploy-preview-1607--detroit-public-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.

@netlify
Copy link

netlify bot commented Mar 28, 2023

Deploy Preview for detroit-partners-dev ready!

Name Link
🔨 Latest commit 7e3fbe6
🔍 Latest deploy log https://app.netlify.com/sites/detroit-partners-dev/deploys/6423880f74f843000884a391
😎 Deploy Preview https://deploy-preview-1607--detroit-partners-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.

@ColinBuyck ColinBuyck changed the title fix: add new strings fix: under construction Mar 28, 2023
@ColinBuyck ColinBuyck marked this pull request as ready for review March 29, 2023 00:43
Comment on lines +218 to +226
let label = t("listings.apply.applicationSeason")
if (listing?.marketingSeason) {
label = label.concat(` ${t(`seasons.${listing.marketingSeason}`)}`)
}
if (listing?.marketingDate) {
label = label.concat(` ${dayjs(listing.marketingDate).year()}`)
}
return label
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think it works for this case in the languages we support, but there is a chance that a language wouldn't have the same order in the sentence.
For example we might say "Residents should apply in Spring 2023" but another language it could be more appropriate to say "In Spring 2023 residents should apply".
I don't think we have to do it for this one, but usually with these translations you should pass the season and year as parameters to the translation string.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@ColinBuyck my assumption would also be to include the message even when no season is listed although @sarahlazarich might have a different recommendation.

Font weight discrepancy seems to be between the figma component and what you have in the code, but lets keep it 500 for consistency with the rest of the application. I added the free font awesome version to the mockup (solid) for access, but if you are using the hollow one consistently, then go ahead and use it here as well. Thanks for double checking!

@sarahlazarich
Copy link
Collaborator

@ColinBuyck - if there are no what to expect translations, I think it's fine to not implement translations here. And I agree with Em's recommendation above.

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.

LGTM (edit: jk QA notes look good!)

@ColinBuyck ColinBuyck merged commit b819b89 into dev Mar 30, 2023
@ColinBuyck ColinBuyck deleted the 1564/under-construction-update branch March 30, 2023 18:06
ludtkemorgan pushed a commit that referenced this pull request Apr 3, 2023
* fix: add new strings

* fix: text and ui implementation

* fix: text and remaining ui implementation

* fix: update metadata formatter

* fix: variable naming cleanup
ludtkemorgan added a commit that referenced this pull request Apr 5, 2023
* fix: under construction (#1607)

* fix: add new strings

* fix: text and ui implementation

* fix: text and remaining ui implementation

* fix: update metadata formatter

* fix: variable naming cleanup

* fix: remove markdown files from prettier

---------

Co-authored-by: ColinBuyck <53269332+ColinBuyck@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants