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.

Sign up for GitHub

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

Jump to bottom

feat: Snow Falling Animation #3535

Merged
merged 13 commits into from
Dec 23, 2024
Merged

feat: Snow Falling Animation #3535

merged 13 commits into from
Dec 23, 2024

Conversation

KonradPietocha
Copy link
Contributor

@KonradPietocha KonradPietocha commented Dec 13, 2024 •
edited
Loading

Description

Changes proposed in this pull request:

  • added react-snowfall library
  • added button to turn the animation off and on
  • using existing feature flag SNOW to enable this feature
  • changed on using const instead of string for feature type

Related issue(s)
#3517
resolves #3517

Definition of done

  • The PR's title starts with one of the following prefixes:
    • feat: A new feature
    • fix: A bug fix
    • docs: Documentation only changes
    • refactor: A code change that neither fixes a bug nor adds a feature
    • test: Adding tests
    • chore: Maintainance changes to the build process or auxiliary tools, libraries, workflows, etc.
  • Related issues are linked. To link internal trackers, use the issue IDs like backlog#4567
  • Explain clearly why you created the PR and what changes it introduces
  • All necessary steps are delivered, for example, tests, documentation, merging

@CLAassistant
Copy link

CLAassistant commented Dec 13, 2024 •
edited
Loading

CLA assistant check
All committers have signed the CLA.

@kyma-bot kyma-bot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. cla: yes Indicates the PR's author has signed the CLA. labels Dec 13, 2024
@KonradPietocha KonradPietocha changed the title feat: Snow Falling Animation #3517 feat: Snow Falling Animation Dec 13, 2024
@mrCherry97 mrCherry97 self-assigned this Dec 17, 2024
mrCherry97
mrCherry97 requested changes Dec 18, 2024
View reviewed changes
Copy link
Contributor

@mrCherry97 mrCherry97 left a comment

Choose a reason for hiding this comment

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

In my opinion we need to decrease number of the snows and change the color on the light theme to be more clear to see. (or we can change image for snow to something like it is on https://minutes.tools.sap/#/ )

Also when I'm enabling snow using button, everything looks good, and snow is falling from top, when I'm connecting the cluster or clicking on something which is causing rerender, when I have a bomb from left top corner.

Screen.Recording.2024-12-18.at.10.26.01.mov

src/header/Header.tsx Outdated Show resolved Hide resolved
src/header/Header.tsx Outdated Show resolved Hide resolved
@KonradPietocha KonradPietocha requested a review from a team as a code owner December 19, 2024 10:14
@kyma-bot kyma-bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Dec 19, 2024
mrCherry97
mrCherry97 requested changes Dec 20, 2024
View reviewed changes
Copy link
Contributor

@mrCherry97 mrCherry97 left a comment

Choose a reason for hiding this comment

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

featureflag on STAGE should be also set on enable, and on prod disabled

mrCherry97
mrCherry97 requested changes Dec 20, 2024
View reviewed changes
Copy link
Contributor

@mrCherry97 mrCherry97 left a comment

Choose a reason for hiding this comment

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

On first render SNOW is disabled, it should be enabled and set in localStorage to enable

mrCherry97
mrCherry97 requested changes Dec 20, 2024
View reviewed changes
src/header/Header.tsx
const handleSnowButtonClick = () => {
if (isSnowOpen) {
setIsSnowOpen(false);
localStorage.setItem(SNOW_STORAGE_KEY, JSON.stringify(false));
Copy link
Contributor

Choose a reason for hiding this comment

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

Can't it be set as boolean?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

localStorage/sessionStorage only likes string variables: "Storage only supports storing and retrieving strings. If you want to save other data types, you have to convert them to strings. For plain objects and arrays, you can use JSON.stringify()." https://developer.mozilla.org/en-US/docs/Web/API/Storage/setItem

@kyma-bot kyma-bot added the lgtm Looks good to me! label Dec 23, 2024
@mrCherry97 mrCherry97 merged commit d96ecb6 into kyma-project:main Dec 23, 2024
15 checks passed
@KonradPietocha KonradPietocha deleted the snowfall-theme branch December 27, 2024 06:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mrCherry97 mrCherry97 mrCherry97 approved these changes

@mmitoraj mmitoraj mmitoraj approved these changes

@akucharska akucharska Awaiting requested review from akucharska akucharska is a code owner

@dbadura dbadura Awaiting requested review from dbadura dbadura is a code owner

@chriskari chriskari Awaiting requested review from chriskari chriskari is a code owner

@OliwiaGowor OliwiaGowor Awaiting requested review from OliwiaGowor OliwiaGowor is a code owner

Assignees

@mmitoraj mmitoraj

@mrCherry97 mrCherry97

Labels
cla: yes Indicates the PR's author has signed the CLA. lgtm Looks good to me! size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Snow Falling Animation
5 participants
@KonradPietocha @CLAassistant @mmitoraj @mrCherry97 @kyma-bot