Skip to content

Add Alert component for showing warnings and errors#1539

Merged
koesie10 merged 3 commits intomainfrom
koesie10/alert-components
Sep 26, 2022
Merged

Add Alert component for showing warnings and errors#1539
koesie10 merged 3 commits intomainfrom
koesie10/alert-components

Conversation

@koesie10
Copy link
Copy Markdown
Member

@koesie10 koesie10 commented Sep 23, 2022

This adds an Alert component which is able to show warnings and errors in the correct styles.

Dark theme

Screenshot 2022-09-23 at 15 24 57

Screenshot 2022-09-23 at 15 25 13

Screenshot 2022-09-23 at 15 25 26

Screenshot 2022-09-23 at 15 25 40

Screenshot 2022-09-23 at 15 27 14

Light theme

Screenshot 2022-09-23 at 15 26 13

Screenshot 2022-09-23 at 15 26 24

Screenshot 2022-09-23 at 15 26 37

Screenshot 2022-09-23 at 15 26 47

Screenshot 2022-09-23 at 15 28 03

Checklist

  • CHANGELOG.md has been updated to incorporate all user visible changes made by this pull request.
  • Issues have been created for any UI or other user-facing changes made by this pull request.
  • [Maintainers only] If this pull request makes user-facing changes that require documentation changes, open a corresponding docs pull request in the github/codeql repo and add the ready-for-doc-review label there.

@koesie10 koesie10 requested a review from a team September 23, 2022 13:28
@koesie10 koesie10 marked this pull request as ready for review September 23, 2022 13:28
@koesie10 koesie10 requested a review from a team as a code owner September 23, 2022 13:28
Copy link
Copy Markdown
Contributor

@robertbrignull robertbrignull left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Component looks great. I'm also going to take some bits as inspiration and copy them over to my PR at #1541

I have one question about colours, and as I say in the comment I'm going to submit a second review focussed on that.

it('renders a warning correctly', () => {
render(<Alert type="warning" title="Warning title" message="Warning content" />);

expect(screen.getByText('warning: Warning title')).toBeInTheDocument();
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't the type be uppercased? Hmm, though I suppose the text content is still lowercase and we're using the styling to display it as uppercase. Is this ok or would it make more sense to do the uppercasing in code rather than styling? If someone copied the text I guess this would mean they'd get the non-styled lowercase version.

Suggested change
expect(screen.getByText('warning: Warning title')).toBeInTheDocument();
expect(screen.getByText('WARNING: Warning title')).toBeInTheDocument();

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is indeed uppercased by the styling, but you're right that when you copy the text it would show as warning. Perhaps it would be even better to use Warning? That would make the most sense when copying this text, such that is shows as e.g. Warning: Access mismatch.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree that capitalisation would be the ideal 👍


const getBackgroundColor = ({ type, inverse }: ContainerProps): string => {
if (!inverse) {
return 'var(--vscode-notifications-background)';
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the colours chosen throughout this file (not just this particular one I've put the comment on) are these the ones that most-closely match the designs? It's unfortunate they don't really form a consistent set, as in we've got ones from notifications, and inputValidation, and debugExceptionWidget.

Does there exist a set we could pick that are meant to go together, and that way it would fit the VSCode colour scheme even though it's slightly different from the designs? What approach have we taken for other components?

I'm going to have a look through for ones I think might fit, but I'll submit those as a separate review so they're all grouped together.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree that it would be better to use standardized color names, but I chose the colors that fit the designs. If we can deviate from the design, I would also prefer to use less colors and standardize on them.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know whether the exact colours need to be adhered to, so long as we have the basic colours and readable text, and colours are either the same or different where they're meant to be. Something that's in my mind is we're optimizing for the standard dark mode but then there are other colour schemes that people could be using.

I'm also aware that changing these colours at a later date will be very easy, so I don't want to hold up our progress here unnecessarily. I'm thinking that for this PR we stick closer to the designs, and I'm happy to go with what in this PR currently, and then we check with @asiermartinez internally on how to approach this question in the future.

Copy link
Copy Markdown
Contributor

@robertbrignull robertbrignull left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Potential suggestions for colour changes. My selection criteria here has been:

  • Use the minimal number of colours. So if we refer to a red in multiple places, then use the same red everywhere.
  • Try to pick a general colour and not one that's meant for a specialised purpose, e.g. preferring editor colours over list-activeSelection.

I'm not trying to say the colours I've picked here are perfect, but I'm interested to hear what you think. I definitely think we should standardise so we aren't using slightly different colours in different modes, or multiples names for the same colour.

Comment thread extensions/ql-vscode/src/view/common/Alert.tsx
Comment thread extensions/ql-vscode/src/view/common/Alert.tsx
Comment thread extensions/ql-vscode/src/view/common/Alert.tsx Outdated
Comment thread extensions/ql-vscode/src/view/common/Alert.tsx
Comment thread extensions/ql-vscode/src/view/common/Alert.tsx Outdated
Comment thread extensions/ql-vscode/src/view/common/Alert.tsx Outdated
Comment thread extensions/ql-vscode/src/view/common/Alert.tsx Outdated
Comment thread extensions/ql-vscode/src/view/common/Alert.tsx Outdated
Comment thread extensions/ql-vscode/src/view/common/Alert.tsx Outdated
@koesie10 koesie10 merged commit 0a5c272 into main Sep 26, 2022
@koesie10 koesie10 deleted the koesie10/alert-components branch September 26, 2022 13:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants