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

App crashes when we remove all colors from scale #10

Closed
pavelkeyzik opened this issue Jun 14, 2022 · 11 comments
Closed

App crashes when we remove all colors from scale #10

pavelkeyzik opened this issue Jun 14, 2022 · 11 comments

Comments

@pavelkeyzik
Copy link
Contributor

What

When I create a new palette and remove all colors from scale, I see a blank page

In Action

CleanShot.2022-06-14.at.23.15.45.mp4

P. S.

What about to add issue template, to let reporters know what they need to provide? 🤔

@colebemis
Copy link
Contributor

Oof good catch. Thanks for the bug report!

Contributions are welcome if you'd like to try and fix this :) Also, feel free to open a pull request to add an issue template for reporting bugs

@pavelkeyzik
Copy link
Contributor Author

I'd love to. But I need some time to learn how everything works 👍

@colebemis
Copy link
Contributor

I'm happy to answer any questions! Also, apologies for the messy code 😅 Lots of opportunities for clean up

@pavelkeyzik
Copy link
Contributor Author

That's okay! The idea is so cool, at least it works and it will be always something to fix. So, don't worry! I hope that community will be involved and help to fix everything 😊

pavelkeyzik added a commit to pavelkeyzik/prism that referenced this issue Jun 15, 2022
I've added check that if colors length is 1, then we disable our button
to let user know that we're not able to remove last color from the
scale and I also updated state machine to ignore all requests to pop
last color from the scale.

Fixes the issue primer#10
pavelkeyzik added a commit to pavelkeyzik/prism that referenced this issue Jun 15, 2022
I've added check that if colors length is 1, then we disable our button
to let user know that we're not able to remove last color from the
scale and I also updated state machine to ignore all requests to pop
last color from the scale.

Fixes the issue primer#10
@rafaeltab
Copy link

I know you're actively developing this now, but I want to note that this error can also be caused by clicking the 'Delete color' button on the right panel. And issue #19 has a very similar error, but when deleting the actively selected color only by clicking the - button, not the 'Delete color' button on the right panel.

@pavelkeyzik
Copy link
Contributor Author

@rafaeltab Good catch 👍

@pavelkeyzik
Copy link
Contributor Author

I'm going to close this issue as it's fixed as part of the #15

@rafaeltab
Copy link

rafaeltab commented Jun 20, 2022

This issue isn't completely solved yet, we can't delete the last color using the - button anymore but when using the Delete color button we can still delete the last color which results in a crash + an errored state.

The export file will look as follows if it becomes errored:

{
  "lightgrey": [],
  "dimgrey": [],
  "olivedrab": "#6b8e23"
}

Both lightgrey and dimgrey are bugged, olivedrab is not. When selecting a bugged scale, the page will also crash, meaning there is no way to delete the bugged scale either.

I believe the functionality that should be added is:

  1. The Delete color button should be unavailable and greyed out when there is only one color in a scale, exactly like the - button.
  2. If a bugged scale is opened, the app should not fully crash but instead display an empty scale, with the scale section of the right bar intact.
  3. (optional) The - and + buttons should still work on a bugged scale

I believe point two is the most important, people need to be able to fix their bugged palettes, instead of having to fully recreate them.

@pavelkeyzik
Copy link
Contributor Author

@rafaeltab I think it'll be better to create a separate issue for this. We're going to fix a part of this issue here #37 but I've realized that user can import scale with empty array and we should add check for this as well 😊

@rafaeltab
Copy link

@pavelkeyzik I was going to make one after your comment on the other issue, but I really didn't have time today since I have to hand in my thesis. Tomorrow I'll be completely free and take the time to make one.

@pavelkeyzik
Copy link
Contributor Author

Thank you @rafaeltab 😊

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

No branches or pull requests

3 participants