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

refactor(notes): change the Note system to reflect what notes are usually used for #547

Closed
wants to merge 1 commit into from

Conversation

maiieul
Copy link
Contributor

@maiieul maiieul commented Dec 8, 2023

What is it?

  • Feature / enhancement
  • Bug
  • Docs / tests

Description

I think this new note system would make more sense. Also, replaced the colors with the background/foreground CSS vars. Check it out, I can't see any color difference!

Checklist:

  • My code follows the developer guidelines of this project
  • I have performed a self-review of my own code
  • I have made corresponding changes to the documentation
  • Added new tests to cover the fix / functionality

@maiieul maiieul changed the title refactor(notes): change the Note system to reflect what notes are usu… refactor(notes): change the Note system to reflect what notes are usually used for Dec 8, 2023
@thejackshelton
Copy link
Collaborator

thejackshelton commented Dec 9, 2023

The naming does not feel intuitive to me at first glance. I'm sure we'd get used to it, but the statuses are likely more expected, similar to how a programmer would expect an API with index in its name to start at the index of 0.

We can refactor success out.

caution or destructive here should be used very sparingly. Let's continue using warn at the introduction, like melt ui does here:
https://www.melt-ui.com/docs/introduction

A destructive action is more concerning, and draws attention away from the introduction.

Where I do think a destructive action makes sense is below. Because if the user decides to use both flip and autoPlacement at the same time, it will break their component. I've purposefully listed this at the bottom, but it has an info tag on it currently.

image

We need a warning status, as there's a difference between something that people should be aware of, and things that are critical.

@maiieul
Copy link
Contributor Author

maiieul commented Dec 10, 2023

Hmmmm, yes, that's trickier than I thought.

This is not a big deal, we can keep it as it was. Didn't spent that much time on this refactor 🙃

Just on a side note:

Melt-UI's "warning" is just their base "blockquote". It's just that their theme happens to be yellow.

I don't see much difference between "warning" and "caution".

Perhaps it would be best to just have Info and Warning? 😬

Although I kind of liked the "Fun Fact" type of note, could be with a ":thinking:" emoji kind of icon.

Also not sure that yellow warning looks very good. Just a warning icon would probably look better.

@maiieul
Copy link
Contributor Author

maiieul commented Dec 10, 2023

Feel free to close if you think it's not worth the trouble 👍

@maiieul
Copy link
Contributor Author

maiieul commented Dec 10, 2023

Yeah let's just close this. You can reopen if you want 👅

@maiieul maiieul closed this Dec 10, 2023
@github-actions github-actions bot locked and limited conversation to collaborators Dec 10, 2023
@thejackshelton
Copy link
Collaborator

I do think a fun fact note wouldn't hurt 😄 . Just that maybe it should be its own status rather than replacing info or warning.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants