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: alert block component [SV-155] #2982

Open
wants to merge 5 commits into
base: v2-develop
Choose a base branch
from

Conversation

FRSgit
Copy link
Contributor

@FRSgit FRSgit commented Sep 22, 2023

Related issue

Closes https://vsf.atlassian.net/browse/SV-155

Scope of work

Block component source code has been added to the Alert page

Screenshots of visual changes

Alert page:
image
image
image

Blocks page:
image
image

Checklist

  • Self code-reviewed
  • Changes documented
  • Semantic HTML
  • SSR-friendly
  • Caching friendly
  • a11y for WCAG 2.0 AA
  • examples created
  • blocks created
  • cypress tests created

@changeset-bot
Copy link

changeset-bot bot commented Sep 22, 2023

⚠️ No Changeset found

Latest commit: 81c2375

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

Copy link
Contributor

@Szymon-dziewonski Szymon-dziewonski left a comment

Choose a reason for hiding this comment

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

Great PR, I have only couple of questions not connected to code itself

I think alert component could be used in
NewsletterBox and OrderSummary since its used there, so we would have better maintaining possibilities, wdyt?

image

Is this Block Code button design comes from figma ?

@FRSgit
Copy link
Contributor Author

FRSgit commented Sep 25, 2023

@Szymon-dziewonski Good idea! Though I'm not sure how to implement it right now. If import Alert block-component in different block then it won't be a "copy-paste" solution "out of the box".

WDYT? (CC: @skirianov)

We had two ideas about working with this case:

  • providing a downloadable ZIP file for any example that consists of more than a single component (ZIP would contain all of the necessary components//files),
  • creating a CLI to do the downloading (maybe we could even prepare some kind of Vite/Webpack solution that could download missing files automatically).

But in any case - I'd do that as a separate step as we need to figure out the solution first

Is this Block Code button design comes from figma ?

Nope, it's my & @skirianov invention - what do you think, acceptable until SFUI gets migrated to new documentation?

@Szymon-dziewonski
Copy link
Contributor

@FRSgit I would we could just point in docs that in source code is use Alert block component which can be found here , however it still does not solve copy paste solution. For me in perfect world would be 2 version of source code e.g NewsletterBox

  1. with component <AlertBlockComponent> so it would work for people that copy paste but already have block alert copied as components, and also for readability so user would see from where it come, and how its connected to point 2
  2. same source code but with upacked code from our block components

With this solution we would have easier maintainability and user could copy whole source code OR source code with componenten and change import for their needs

As for styling, I would go with darker green and rounded border (top-left, top-right), but can be like that as well

@FRSgit
Copy link
Contributor Author

FRSgit commented Sep 25, 2023

@Szymon-dziewonski yea, that solution would work as well - but I'd implement it in new docs rather then in vuepress. Don't want to waste time on something that should be removed 😄

BTW. @mattmaribojoc @skirianov @filrak do we any ETA on docs replacement for SFUI?

@sonarcloud
Copy link

sonarcloud bot commented Sep 27, 2023

[storefront-ui-vue] Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

100.0% 100.0% Coverage
0.0% 0.0% Duplication

warning The version of Java (11.0.20) you have used to run this analysis is deprecated and we will stop accepting it soon. Please update to at least Java 17.
Read more here

@sonarcloud
Copy link

sonarcloud bot commented Sep 27, 2023

[storefront-ui-react] Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

warning The version of Java (11.0.20) you have used to run this analysis is deprecated and we will stop accepting it soon. Please update to at least Java 17.
Read more here

@skirianov
Copy link

@Szymon-dziewonski Good idea! Though I'm not sure how to implement it right now. If import Alert block-component in different block then it won't be a "copy-paste" solution "out of the box".

WDYT? (CC: @skirianov)

We had two ideas about working with this case:

  • providing a downloadable ZIP file for any example that consists of more than a single component (ZIP would contain all of the necessary components//files),
  • creating a CLI to do the downloading (maybe we could even prepare some kind of Vite/Webpack solution that could download missing files automatically).

But in any case - I'd do that as a separate step as we need to figure out the solution first

With regards to zip, I would personally go for the CLI (shadcn/ui is a great example), because working with zips sucks.

@FRSgit
Copy link
Contributor Author

FRSgit commented Sep 27, 2023

@skirianov yea, true! My main thought was - let's worry about that in a separate channel, not under this PR 😄

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