search

Melkeydev / astrostation

https://astrostation.me
MIT License
209 stars 42 forks source link

added color selector to sticky note componenets #236

Closed herropaul closed 1 year ago

herropaul commented 1 year ago

https://github.com/MelkeyOSS/astrostation/issues/197

now able to select a few different colors of a sticky note. Although the functionality works, do we have any specific colorways we would want to add?

vercel[bot] commented 1 year ago

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated
astrostation ✅ Ready (Inspect) Visit Preview 💬 Add your feedback Mar 28, 2023 at 4:06AM (UTC)
herropaul commented 1 year ago

Whoaa! Looks tight! 🥰

Two minor comments:

  1. It looks like the package-lock.json file got updated behind the scenes. I think that's only supposed to get updated in response to either the node_modules tree or package. json file being updated. I'd remove that file change before merging.
  2. One small gripe with your changes -- I love this addition, but for whatever reason if I have to close Astrostation and re-open, the color of the sticky note is lost. I would add a new field to cache the sticky note color via interfaces.ts and implement it into a constant inside of the store for use within Sticky.tsx. Let me know if you need help using the pattern.

You know, i totally forgot about persisting the state for the color selected, thank you for bringing that up haha. I'll try to update the changes with that pattern and I'll let you know if i have any questions 👍

mastermajorman commented 1 year ago

Whoaa! Looks tight! 🥰 Two minor comments:

  1. It looks like the package-lock.json file got updated behind the scenes. I think that's only supposed to get updated in response to either the node_modules tree or package. json file being updated. I'd remove that file change before merging.
  2. One small gripe with your changes -- I love this addition, but for whatever reason if I have to close Astrostation and re-open, the color of the sticky note is lost. I would add a new field to cache the sticky note color via interfaces.ts and implement it into a constant inside of the store for use within Sticky.tsx. Let me know if you need help using the pattern.

You know, i totally forgot about persisting the state for the color selected, thank you for bringing that up haha. I'll try to update the changes with that pattern and I'll let you know if i have any questions 👍

Sounds good, thank you for the contribution!

Oh, I am wrong on that package-lock comment. It looks like all those are third-party updates besides the local version update in the file. Please include this change.

royanger commented 1 year ago

@herropaul Separate from any review, please edit the PR so that it is against the staging branch. Please also update contributors.yml. Thanks.

Melkeydev commented 1 year ago

When you make the changes @me so i can review again - this looks awesome. Great PR. @herropaul

herropaul commented 1 year ago

When you make the changes @me so i can review again - this looks awesome. Great PR. @herropaul

Yessir! appreciate it man!