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

feat(SaleHeaderBanner): L3-4648 add countdown to sale header banner f… #435

Merged

Conversation

aahill50
Copy link
Contributor

@aahill50 aahill50 commented Nov 25, 2024

…or timed auctions

Jira ticket

L3-4648

Screenshots

Before After
L3-4648 before L3-4648 after
L3-4648 mobile before L3-4648 mobile after

Summary

Adds the countdown timer to the top of Time Auction Sale pages

Change List (describe the changes made to the files)

  • Additions, Changes, and Removals

Acceptance Test (how to verify the PR)

  • Add step by step instructions on how to verify the change

Regression Test

  • (Optional) Add verification steps to make sure this PR doesn't break old functionality

Evidence of testing

  • Post logs, screenshots, etc

Things to look for during review

  • PR title should correctly describe the most significant type of commit. I.e. feat(scope): ... if a minor release should be triggered.
  • All commit messages follow convention and are appropriate for the changes
  • All references to phillips class prefix are using the prefix variable
  • All major areas have a data-testid attribute.
  • Document all props with jsdoc comments
  • All strings should be translatable.
  • Unit tests should be written and should have a coverage of 90% or higher in all areas.

New Components

  • Are there any accessibility considerations that need to be taken into account and tested?
  • Default story called "Playground" should be created for all new components
  • Create a jsdoc comment that has an Overview section and a link to the Figma design for the component
  • Export the component and its typescript type from the index.ts file
  • Import the component scss file into the componentStyles.scss file.

Copy link

netlify bot commented Nov 25, 2024

Deploy Preview for phillips-seldon ready!

Name Link
🔨 Latest commit 976cd6d
🔍 Latest deploy log https://app.netlify.com/sites/phillips-seldon/deploys/674f2412712fd900085a34fe
😎 Deploy Preview https://deploy-preview-435--phillips-seldon.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.

@aahill50 aahill50 force-pushed the L3-4648-update-sale-header-banner-to-support-timed-auctions branch from 557a96d to f59696a Compare November 26, 2024 15:19
/>
<PageMargin className={`${baseClassName}__stack-wrapper`} {...commonProps} {...props} ref={ref}>
{showCountdown && (
<Countdown endDateTime={new Date(auctionEndTime)} className={`${baseClassName}__countdown`} />
Copy link
Collaborator

Choose a reason for hiding this comment

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

I believe they moved the countdown timer above the banner image on mobile. Not sure why Figma is showing both above and below. We can utilize the media query util we use in other components like footer.

Copy link
Contributor Author

@aahill50 aahill50 Nov 27, 2024

Choose a reason for hiding this comment

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

Weird, I think it was only on the bottom when I first started working on it, but now I see it in both places in figma... But I just pushed up a commit that moves it to the top and gets rid of the bottom border for the mobile layout

@@ -86,7 +97,11 @@ const SaleHeaderBanner = forwardRef<HTMLDivElement, SaleHeaderBannerProps>(
/>
<PageMargin className={`${baseClassName}__stack-wrapper`} {...commonProps} {...props} ref={ref}>
<div className={`${baseClassName}__stack`}>
{isOpenForBidding && children}
{isOpenForBidding && auctionEndTime && (
Copy link
Contributor

Choose a reason for hiding this comment

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

apparently we prefer doing this

{isOpenForBidding && auctionEndTime ? (<Component />) : null}

but I think it's ok if there are no numbers involved
sometimes you get weird stuff where if auctionEndTIme was 0, it would just render a 0 on the page
but that shouldn't happen here

Copy link
Contributor Author

@aahill50 aahill50 Dec 2, 2024

Choose a reason for hiding this comment

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

That's usually how I prefer to do it, but I was just following the convention that was already in the file... but since you mentioned it, I just pushed up a refactor commit to change all the conditional rendering in this file to ternarys

@aahill50 aahill50 merged commit 716701e into main Dec 3, 2024
12 checks passed
@aahill50 aahill50 deleted the L3-4648-update-sale-header-banner-to-support-timed-auctions branch December 3, 2024 17:05
davidicus pushed a commit that referenced this pull request Dec 3, 2024
# [1.99.0](v1.98.1...v1.99.0) (2024-12-03)

### Features

* **SaleHeaderBanner:** L3-4648 add countdown to sale header banner f… ([#435](#435)) ([716701e](716701e))
@davidicus
Copy link
Collaborator

🎉 This issue has been resolved in version 1.99.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants