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.

Sign up for GitHub

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

Jump to bottom

1713/remove business logic from AppStatusItem component #1714

Merged
merged 7 commits into from
Sep 13, 2021
Merged

1713/remove business logic from AppStatusItem component #1714

merged 7 commits into from
Sep 13, 2021

Conversation

emilyjablonski
Copy link
Collaborator

@emilyjablonski emilyjablonski commented Aug 24, 2021 •
edited
Loading

Pull Request Template

Issue

Addresses #1713

  • 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

Removes business logic from AppStatusItem component. Included removing our URL formatting. Also fixes a bug where the component would show Application Due Date: Invalid Date if the date was null (any FCFS listing) - now I just don't show that piece if it's null.

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Prototype/POC (not to merge)
  • This change is a refactor/address technical debt
  • This change requires a documentation update
  • This change requires a SQL Script

How Can This Be Tested/Reviewed?

You can look in storybook, and in the public app the AppStatusItem component is used in the My Applications tab.

Checklist:

  • My code follows the style guidelines of this project
  • 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 updated the changelog to include a description of my changes

@netlify
Copy link

netlify bot commented Aug 24, 2021 •
edited
Loading

✔️ Deploy Preview for dev-storybook-bloom ready!

🔨 Explore the source changes: 8dd43e5

🔍 Inspect the deploy log: https://app.netlify.com/sites/dev-storybook-bloom/deploys/613fa1d2b54d4000088a979a

😎 Browse the preview: https://deploy-preview-1714--dev-storybook-bloom.netlify.app

@netlify
Copy link

netlify bot commented Aug 24, 2021 •
edited
Loading

✔️ Deploy Preview for dev-partners-bloom ready!

🔨 Explore the source changes: 8dd43e5

🔍 Inspect the deploy log: https://app.netlify.com/sites/dev-partners-bloom/deploys/613fa1d2008b3900071c3994

😎 Browse the preview: https://deploy-preview-1714--dev-partners-bloom.netlify.app

@netlify
Copy link

netlify bot commented Aug 24, 2021 •
edited
Loading

✔️ Deploy Preview for dev-bloom ready!

🔨 Explore the source changes: 8dd43e5

🔍 Inspect the deploy log: https://app.netlify.com/sites/dev-bloom/deploys/613fa1d250982000073caae1

😎 Browse the preview: https://deploy-preview-1714--dev-bloom.netlify.app

@emilyjablonski emilyjablonski marked this pull request as ready for review August 24, 2021 16:33
@emilyjablonski emilyjablonski changed the title 1713/remove business logic from appStatusItem component 1713/remove business logic from AppStatusItem component Aug 24, 2021
Base automatically changed from dev to master August 25, 2021 17:59
@emilyjablonski emilyjablonski changed the base branch from master to dev August 25, 2021 18:02
seanmalbert
seanmalbert requested changes Aug 27, 2021
View reviewed changes
Copy link
Collaborator

@seanmalbert seanmalbert left a comment

Choose a reason for hiding this comment

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

Hi @emilyjablonski , this looks good. I have a question around how dates should be passed in, which is up for discussion. My opinion is that dates should be passed into the component already formatted, allowing SF, the core team or anyone else to use the date library of their choice.

ui-components/src/blocks/AppStatusItem.tsx Outdated
Comment on lines 27 to 26
{t("listings.applicationDeadline")}: {applicationDueDate.format("MMMM D, YYYY")}
</p>
Copy link
Collaborator

Choose a reason for hiding this comment

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

What do you think about passing in dates that are already formatted? I think we should work towards UI Components being independent of date libraries like Moment.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Great idea 👍

@netlify
Copy link

netlify bot commented Aug 27, 2021 •
edited
Loading

✔️ Deploy Preview for clever-edison-cd22c1 ready!

🔨 Explore the source changes: e0b203c

🔍 Inspect the deploy log: https://app.netlify.com/sites/clever-edison-cd22c1/deploys/612fb7100ca6680007c7cb44

😎 Browse the preview: https://deploy-preview-1714--clever-edison-cd22c1.netlify.app

jaredcwhite
jaredcwhite requested changes Aug 31, 2021
View reviewed changes
Copy link
Collaborator

@jaredcwhite jaredcwhite left a comment

Choose a reason for hiding this comment

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

Just one deprecation to look at and a general question, otherwise looks good to me. 👍

ui-components/src/blocks/AppStatusItem.tsx Outdated
className="status-item__link lined"
href={`/listing/${listing.id}/${listing.urlSlug}`}
>
<LocalizedLink className="status-item__link lined" href={props.listingURL}>
Copy link
Collaborator

Choose a reason for hiding this comment

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

FYI, LocalizedLink is deprecated and we should probably remove it soon. Best to use LinkComponent from the NavigationContext instead. (See the LinkButton component for example usage.)

ui-components/src/blocks/AppStatusItem.tsx Outdated
applicationUpdatedAt: string
confirmationNumber?: string
listingName: string
listingURL: string
}

const AppStatusItem = (props: AppStatusItemProps) => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I wonder if we should rename this and the props as well to something even more generic like just StatusItem. That's what the CSS class names are already.

seanmalbert
seanmalbert approved these changes Sep 9, 2021
View reviewed changes
Copy link
Collaborator

@seanmalbert seanmalbert 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 to me, @jaredcwhite , can you take another look since the updates.

@emilyjablonski emilyjablonski linked an issue Sep 9, 2021 that may be closed by this pull request
Remove business logic from AppStatusItem #1713
Closed
@emilyjablonski
Copy link
Collaborator Author

@jaredcwhite

jaredcwhite
jaredcwhite approved these changes Sep 13, 2021
View reviewed changes
Copy link
Collaborator

@jaredcwhite jaredcwhite left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@emilyjablonski emilyjablonski merged commit 4612e93 into dev Sep 13, 2021
@emilyjablonski emilyjablonski deleted the 1713/appstatusitem branch September 13, 2021 21:12
seanmalbert pushed a commit to CityOfDetroit/bloom that referenced this pull request Jun 23, 2022
@emilyjablonski
Merge pull request bloom-housing#1714 from bloom-housing/1713/appstat…
64bf984 
…usitem
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jaredcwhite jaredcwhite jaredcwhite approved these changes

@seanmalbert seanmalbert seanmalbert approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Remove business logic from AppStatusItem
3 participants
@emilyjablonski @jaredcwhite @seanmalbert