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

Tweak emoji placement in rule doc config notice when combination of enabled/warn/disabled present #310

Open
bmish opened this issue Nov 29, 2022 · 3 comments · May be fixed by #311
Open
Labels
bug Something isn't working

Comments

@bmish
Copy link
Owner

bmish commented Nov 29, 2022

Received feedback that the two leading emojis in this example rule doc configs notice could be confusing (jsx-eslint/eslint-plugin-react#3499 (comment)).

Proposal 1

We could move the emojis next to their respective sentences to keep this as a consolidated notice.

Pros:

  • Keep to a single line for minimizing vertical space needed
  • More consistent with our other consolidated fixableAndHasSuggestions notice

Cons:

  • The notice's leading emoji doesn't fully represent the contents of the line anymore

Proposal 2

We could split these into separate notices/lines, similar to how we separated the rule list configs column into multiple columns based on severity.

Pros:

  • Maximize clarity
  • Maximize aesthetics
  • Avoids attempting to convey multiple distinct pieces of information in a single line

Cons:

  • Uses additional vertical space
  • Does not give the user the option to choose whether they want the severities consolidated into one line or split into multiple lines

Proposal 3

Split configs notice into configsError,configsWarn,configsOff, same as how we did for the configs column, so the user can control exactly where these notices go.

Pros:

  • Same benefits as Proposal 2
  • Allows us to keep the configs notice type for people who want to consolidate all the severities into a single line, while having separate notice types for those who want to use separate lines (similar to fixableAndHasSuggestions vs fixable and hasSuggestions)

Cons:

  • Same benefits as Proposal 2
  • More added complexity having to manage these notices separately instead of all together
  • If we want to change the default behavior of this, it would require a breaking change
  • Possibly overkill for now and we can switch to this later if there's a need

(current)

💼🚫 This rule is enabled in the ☑️ recommended config. This rule is disabled in the jsx-runtime config.


(Proposal 1: consolidated with emojis next to corresponding sentence)

💼 This rule is enabled in the ☑️ recommended config. 🚫 This rule is disabled in the jsx-runtime config.


(Proposal 2/3: separate lines)

💼 This rule is enabled in the ☑️ recommended config.

🚫 This rule is disabled in the jsx-runtime config.

@ljharb
Copy link

ljharb commented Nov 29, 2022

Why not separate lines? Adding a newline doesn't seem that complex.

@bmish
Copy link
Owner Author

bmish commented Nov 29, 2022

@ljharb I've implemented separate lines (proposal 2 in the issue description) in #311. The implementation is trivial. The situation would have been more complex had I gone with Proposal 3 which would give the user the flexibility to choose whether they want to have everything on a single line or separate lines, but that's probably overkill for now.

@bmish
Copy link
Owner Author

bmish commented Dec 1, 2022

I'm going to ponder on this a bit more to decide if I want to bite the bullet and try out Proposal 3 for better future-proofing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants