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

[Feature] Update browser extension to support new severity values #3181

Closed
molant opened this issue Oct 24, 2019 · 24 comments · Fixed by #3340
Closed

[Feature] Update browser extension to support new severity values #3181

molant opened this issue Oct 24, 2019 · 24 comments · Fixed by #3340

Comments

@molant
Copy link
Member

@molant molant commented Oct 24, 2019

As part of #3036 (and follow up of #3070 and #3071) we need to update the browser extension to report new severity values.

Right now it only supports Error and Warning but we are going to have Hint and Information as well.

Also we will need to allow the user to set the threshold.

This requires design and implementation work so assigning to @ststimac.

@molant molant changed the title [Feature] Feature description [Feature] Update browser extension to support new severity values Oct 24, 2019
@molant molant added this to the 1911-1 milestone Oct 25, 2019
@ststimac

This comment has been minimized.

Copy link
Member

@ststimac ststimac commented Nov 4, 2019

@molant @hxlnt @antross what do we think of doing labels with an icon for different severity types.
These are the icons I'm proposing but this does not encompass the whole label component of icon + text.
Screen Shot 2019-11-04 at 3 33 43 PM

@antross

This comment has been minimized.

Copy link
Member

@antross antross commented Nov 4, 2019

what do we think of doing labels with an icon for different severity types.

I like it. Where do you think we'd put the icons, before individual messages?
image

I ask because the "Hints: 8" text in that screenshot that indicates a warning may be too general for a single icon because many hints will now report messages of differing severities (which probably means we'll need to rethink the "Hints: 8" formatting slightly too...)

@ststimac

This comment has been minimized.

Copy link
Member

@ststimac ststimac commented Nov 4, 2019

@antross that's what I was thinking. I was thinking similar to the badges we do already but different colors. I can mock it up.

I was thinking: yellow for warning, red for error, webhint purple for hints, and maybe grey for information? I was originally thinking blue but don't want that to get lost.

@ststimac

This comment has been minimized.

Copy link
Member

@ststimac ststimac commented Nov 5, 2019

So something like this:
Screen Shot 2019-11-05 at 10 30 27 AM

@antross

This comment has been minimized.

Copy link
Member

@antross antross commented Nov 5, 2019

The badges look nice, though the start of the message text being shifted by different amounts feels slightly odd to me, especially for "Information". Curious to hear what others think.

Also, what do you think about the existing summaries, e.g. "Hints: 2" and the top-level colored dots for categories? Should we surface something about the differing contained severities at those levels too? The text "Hints: 2" itself will probably be out of place now that we have a distinct "Hint" severity.

@ststimac

This comment has been minimized.

Copy link
Member

@ststimac ststimac commented Nov 5, 2019

Yeah that's a good point about the Hints: 2 badge at the top. I think some sort of indicator with the dots would be good, to indicate there's an issue. Let me try a few things.

As for the shifting of the message, that doesn't feel off to me but am open to others opinions.

@ststimac

This comment has been minimized.

Copy link
Member

@ststimac ststimac commented Nov 5, 2019

Screen Shot 2019-11-05 at 12 01 39 PM

@antross, I think if we drop the badge and bold the lettering that should be sufficient.

@molant

This comment has been minimized.

Copy link
Member Author

@molant molant commented Nov 5, 2019

Not convinced about having Hints: 2 and then another Hint right after.

Also, right now we have another badge with "No issues" that should probably be redesigned to be consistent with the rest.

Maybe having a summary of the results there? Something like"Errors: 3, Warnings: 2" and remove the categories that don't have any result or that do not meet the threshold?

That has the risk of making the title pretty long...

Or maybe we only have a message "No issues" or a ✔ and nothing when something has been found?

@ststimac

This comment has been minimized.

Copy link
Member

@ststimac ststimac commented Nov 7, 2019

I like the summary of issues idea and removing the categories that don't have results.

No issues would probably just be a green badge with a check icon to match the other badges.

@ststimac

This comment has been minimized.

Copy link
Member

@ststimac ststimac commented Nov 7, 2019

Also for the config panel, I suggest we add another column with checkboxes for the severity types.

@molant

This comment has been minimized.

Copy link
Member Author

@molant molant commented Nov 7, 2019

I suggest we add another column with checkboxes for the severity types.

I see it more as a "minimum threshold". I don't think why someone would just select "warning" but not "error" and I'm not even sure we should enable this scenario.
So basically the user needs to be able to decide what's the minimum type of report they want to receive:

  1. Error
  2. Warning
  3. Information
  4. Hint

Personally I see it more as a slider but I'm fine as long as we don't allow things like "I want hints and warnings only"

@ststimac

This comment has been minimized.

Copy link
Member

@ststimac ststimac commented Nov 7, 2019

If that's the case, a radio button would probably be the better solution.

@ststimac

This comment has been minimized.

Copy link
Member

@ststimac ststimac commented Nov 8, 2019

@antross @molant what else do you need from me before I head to btconf next Tuesday?

@molant

This comment has been minimized.

Copy link
Member Author

@molant molant commented Nov 8, 2019

Can we have the design of the settings and the final look and feel of the results?

@ststimac

This comment has been minimized.

Copy link
Member

@ststimac ststimac commented Nov 8, 2019

Config panel. But we should discuss the title for the new settings. I'm not sure it's super clear what this will mean to folks. I have an idea for a different layout that would include a small caption to identify what this setting is for.

Screen Shot 2019-11-08 at 11 37 48 AM

@ststimac

This comment has been minimized.

Copy link
Member

@ststimac ststimac commented Nov 8, 2019

Okay: for the results, I'm thinking we don't need actually need a green badge because we don't have a subset of items for those categories that have no issues. So I'm proposing we just keep "No issues" at the top level but match the styling of the error, warning message summary. Make sense?

Screen Shot 2019-11-08 at 2 26 51 PM

@molant molant modified the milestones: 1911-1, 1911-2 Nov 11, 2019
@sarvaje

This comment has been minimized.

Copy link
Member

@sarvaje sarvaje commented Nov 13, 2019

Is the result page ready to implement?

@molant

This comment has been minimized.

Copy link
Member Author

@molant molant commented Nov 13, 2019

@hxlnt

This comment has been minimized.

Copy link
Member

@hxlnt hxlnt commented Nov 13, 2019

Liking the latest revisions. Within a given tree, do we want the sort order to be all errors, then all warnings, then all hints, and then all informations?

@molant

This comment has been minimized.

Copy link
Member Author

@molant molant commented Nov 13, 2019

@sarvaje sarvaje mentioned this issue Nov 13, 2019
2 of 4 tasks complete
@sarvaje sarvaje self-assigned this Nov 14, 2019
@molant

This comment has been minimized.

Copy link
Member Author

@molant molant commented Nov 15, 2019

@ststimac can we pretty please have the icons as svgs files? 🙏
Is the only thing missing to merge #3340

@ststimac

This comment has been minimized.

Copy link
Member

@ststimac ststimac commented Nov 15, 2019

icons.zip

@molant here ya go

@molant

This comment has been minimized.

Copy link
Member Author

@molant molant commented Nov 15, 2019

Thanks a lot!

@sarvaje

This comment has been minimized.

Copy link
Member

@sarvaje sarvaje commented Nov 15, 2019

Thanks @ststimac!!

@molant molant closed this in #3340 Nov 18, 2019
molant added a commit that referenced this issue Nov 18, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.