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

STUD-381: Add feature flag to disable song minting and distribution #831

Merged
merged 2 commits into from
Nov 21, 2024

Conversation

escobarjonatan
Copy link
Contributor

@escobarjonatan escobarjonatan commented Nov 21, 2024

image

@escobarjonatan escobarjonatan requested a review from a team as a code owner November 21, 2024 04:44
Copy link

nx-cloud bot commented Nov 21, 2024

☁️ Nx Cloud Report

CI is running/has finished running commands for commit 8635703. As they complete they will appear below. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this CI Pipeline Execution


✅ Successfully ran 1 target

Sent with 💌 from NxCloud.

Copy link

Visit the studio preview URL for this PR 🚀 : https://831.artist.preview.newm.io/

Copy link

Visit the tools preview URL for this PR (updated for commit 7e37089):
🚀 https://k33asgbrtc4eznccgjzrtyaga40vwszr.lambda-url.us-west-2.on.aws/

Copy link

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

Copy link

Visit the marketplace preview URL for this PR (updated for commit 7e37089):
🚀 https://cxsizequmqpi5a2ujeibjjemhy0ifbou.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.

Overall looks good, one minor logic issue spotted on edit page and another optional UI change/inquiry

Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like the Switch element is supposed to change to grey/opacity to .7 when disabled, but I'm not sure if we want to also engage disabled view for the icon. Still shows as a white circle when disabled currently.

Could open a new ticket for this one, won't block this PR on account to this.

"&.Mui-disabled + .MuiSwitch-track": {
opacity: 0.7,
},
"&.Mui-disabled .MuiSwitch-thumb": {
color: theme.colors.grey100,
},

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yea this can be its own ticket

Comment on lines +413 to +419
toggleTooltipText={
webStudioDisableTrackDistributionAndMinting
? "Track distribution and minting is temporarily disabled. " +
"Please upload your song to save your progress, and check " +
"back later to finish the distribution process."
: undefined
}
Copy link
Contributor

Choose a reason for hiding this comment

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

This shows up in the Library edit song page when the minting is already toggled by default, I'm able to even proceed to next, logic needs to be added to disable this route as well.

image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good catch, fixed it

Copy link
Contributor

Choose a reason for hiding this comment

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

Fix looks good. We will need to make it more obvious though, first instinct isn't to go over to minting toggle to check for tooltip as the reason Next button is disabled, might confuse users.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can wait on feedback for this as it should just be in for a short(hopefully) period of time

Copy link

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

Copy link

Visit the tools preview URL for this PR (updated for commit 5ea93d1):
🚀 https://k33asgbrtc4eznccgjzrtyaga40vwszr.lambda-url.us-west-2.on.aws/

Copy link

Visit the marketplace preview URL for this PR (updated for commit 5ea93d1):
🚀 https://cxsizequmqpi5a2ujeibjjemhy0ifbou.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.

Fix looks good to disable minting, one comment left for UX

Comment on lines +413 to +419
toggleTooltipText={
webStudioDisableTrackDistributionAndMinting
? "Track distribution and minting is temporarily disabled. " +
"Please upload your song to save your progress, and check " +
"back later to finish the distribution process."
: undefined
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Fix looks good. We will need to make it more obvious though, first instinct isn't to go over to minting toggle to check for tooltip as the reason Next button is disabled, might confuse users.

@escobarjonatan escobarjonatan merged commit ed94d3d into master Nov 21, 2024
6 checks passed
@escobarjonatan escobarjonatan deleted the STUD-381-add-flag-to-disable-mint branch November 21, 2024 16:10
Copy link

💀 wallet preview environment destroyed

Copy link

💀 tools preview environment destroyed

Copy link

💀 marketplace preview environment destroyed

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