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

Misc fixes #633

Merged
merged 5 commits into from
May 21, 2024
Merged

Misc fixes #633

merged 5 commits into from
May 21, 2024

Conversation

scandycuz
Copy link
Contributor

  • Updates song card price layout to stack vertically.
  • limits "More from {artist}" and "Similar songs" to 8 songs to not make it difficult to scroll to other sections. Post-MVP, some sort of "View all" button can be added.

Copy link

github-actions bot commented May 19, 2024

Visit the preview URL for this PR (updated for commit ad11303):

https://fan-square--pr633-hotfix-song-card-pri-dukgxprx.web.app

(expires Thu, 20 Jun 2024 21:59:09 GMT)

🔥 via Firebase Hosting GitHub Action 🌎

Sign: b38f4676e459f355dcb11c95730012d43389bfd2

Copy link

github-actions bot commented May 19, 2024

Visit the preview URL for this PR (updated for commit ad11303):

https://newm-artist-portal--pr633-hotfix-song-card-pri-9rsiw6oe.web.app

(expires Thu, 20 Jun 2024 21:59:09 GMT)

🔥 via Firebase Hosting GitHub Action 🌎

Sign: ec3483d4c62309afc398865bfd6a9fc9e03e1d46

Copy link

Visit the marketplace preview URL for this PR (updated for commit 6927a07):
🚀 https://whtdxsrp7o3qsprom3zxfoosf40jvudc.lambda-url.us-west-2.on.aws/

Copy link

Visit the wallet preview URL for this PR (updated for commit 6927a07):
🚀 https://pawf6kharlnbpc7laomvqi5cca0gkglx.lambda-url.us-west-2.on.aws/

Copy link
Contributor

@dmkirshon dmkirshon left a comment

Choose a reason for hiding this comment

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

LGTM, just minor comment

Copy link
Contributor

Choose a reason for hiding this comment

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

The ItemSkeleton.tsx, (which probably should be renamed to SaleSkeleton.tsx) also doesn't match up to the new price layout.

const ItemSkeleton: FunctionComponent = () => (

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It looks like the price should actually still appear in the upper right corner for the actual item. I'll make a separate ticket to make that update, since that requirement wasn't clear when changing the way it appeared for the previews.

@dmkirshon
Copy link
Contributor

Noticed this through my review as well but not directly related, must have been a recent PR. Saw an error with the Sale type being imported for the Sale component. Minor conflict but probably should be either refactored with a renaming of the two or at least be updated to the following to remove the error:

import type { Sale } from "../modules/sale";

import { Sale } from "../modules/sale";
interface SaleProps {
readonly isLoading: boolean;
readonly sale?: Sale;
}
const Sale: FunctionComponent<SaleProps> = ({ sale, isLoading }) => {

@scandycuz
Copy link
Contributor Author

Resolved the import name conflict.

Copy link

Visit the wallet preview URL for this PR (updated for commit c47f6b2):
🚀 https://pawf6kharlnbpc7laomvqi5cca0gkglx.lambda-url.us-west-2.on.aws/

Copy link

Visit the marketplace preview URL for this PR (updated for commit c47f6b2):
🚀 https://whtdxsrp7o3qsprom3zxfoosf40jvudc.lambda-url.us-west-2.on.aws/

@scandycuz scandycuz merged commit 8b0aeda into master May 21, 2024
7 checks passed
@scandycuz scandycuz deleted the hotfix/song-card-price-layout branch May 21, 2024 22:11
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.

3 participants