-
Notifications
You must be signed in to change notification settings - Fork 7k
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
fix: Cal 3939 fix org banner #15504
fix: Cal 3939 fix org banner #15504
Conversation
Someone is attempting to deploy a commit to the cal Team on Vercel. A member of the Team first needs to authorize it. |
Hey there and thank you for opening this pull request! 👋🏼 We require pull request titles to follow the Conventional Commits specification and it looks like your proposed title needs to be adjusted. Details:
|
🚨 Potential security issues detected. Learn more about Socket for GitHub ↗︎ To accept the risk, merge this PR and you will not be notified again.
Next stepsWhat is an install script?Install scripts are run when the package is installed. The majority of malware in npm is hidden in install scripts. Packages should not be running non-essential scripts during install and there are often solutions to problems people solve with install scripts that can be run at publish time instead. What is telemetry?This package contains telemetry which tracks how it is used. Most telemetry comes with settings to disable it. Consider disabling telemetry if you do not want to be tracked. Take a deeper look at the dependencyTake a moment to review the security alert above. Review the linked package source code to understand the potential risk. Ensure the package is not malicious before proceeding. If you're unsure how to proceed, reach out to your security team or ask the Socket team for help at support [AT] socket [DOT] dev. Remove the packageIf you happen to install a dependency that Socket reports as Known Malware you should immediately remove it and select a different dependency. For other alert types, you may may wish to investigate alternative packages or consider if there are other ways to mitigate the specific risk posed by the dependency. Mark a package as acceptable riskTo ignore an alert, reply with a comment starting with
|
Graphite Automations"Add community label" took an action on this PR • (06/20/24)1 label was added to this PR based on Keith Williams's automation. "Add foundation team as reviewer" took an action on this PR • (06/20/24)1 reviewer was added to this PR based on Keith Williams's automation. "Add consumer team as reviewer" took an action on this PR • (06/20/24)1 reviewer was added to this PR based on Keith Williams's automation. |
8748ee1
to
c893349
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work so far @Preetam078. I would love to see some more even spacing when a banner isn't uploaded. It would also be great to see a placeholder banner when there isn't a banner set.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we're passing isBannerAvatar
we should create a new BannerAvatar
component. Thoughts @sean-brydon?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hii @joeauyeung is there any suggestions from your side which I can use as the placeholder may be like a Fallback image or a simple html container to control the CLS (content load shifting)
in case there is no banner image is uploaded, Thoughts @sean-brydon ? also let me know if there is a separate component required for BannerAvatar
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah i'd suggest these being seperate components i think. They're pretty different from one another
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sean-brydon what about the placeholder, if in case there is no banner uploaded ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hey @joeauyeung, @sean-brydon I have created a separate component for BannerAvatar in my local machine. Just wanted to know if their is any suggestion you guys have for placeholder for banner before PR update ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Preetam078 it looks like you've pushed the changes for the |
738b094
to
3807e33
Compare
What does this PR do?
Fixes #15456
Fixes CAL-3939 (fix org banner)
This PR implemented the new UI for Banner upload in the Organisation profile.
https://www.loom.com/share/ede071daea1b4b2bad062672ded346b2
Since we cannot directly access the organisation profile page thats why have mimic the changes in the Team Profile page, (These are the changes which i build for the Organisation profile). for More clarity please go through the code implementation.
Mandatory Tasks (DO NOT REMOVE)
How should this be tested?