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

[Alert] Add new component #18094

Closed
1 task done
dimitropoulos opened this issue Oct 29, 2019 · 39 comments · Fixed by #18702
Closed
1 task done

[Alert] Add new component #18094

dimitropoulos opened this issue Oct 29, 2019 · 39 comments · Fixed by #18702
Assignees
Labels
component: alert This is the name of the generic UI component, not the React module! new feature New feature or request

Comments

@dimitropoulos
Copy link
Member

dimitropoulos commented Oct 29, 2019

relevant PR: #18702

I am happy to contribute this component. I am making this issue to determine interest and discuss the scope of the component.

This issue is to track the development in an Alert component. This component provides contextual feedback messages for typical user actions.

Summary 💡

This component provides contextual feedback messages when you need a persistent static container which is closable by user actions or when you need to show alert messages to users.

Examples 🌈

Many libraries have implementations of this:

Motivation 🔦

As with the examples above, I have used such a component before in 3 of the above mentioned solutions and find it missing in material-ui.

@oliviertassinari oliviertassinari added the component: alert This is the name of the generic UI component, not the React module! label Oct 29, 2019
@oliviertassinari
Copy link
Member

oliviertassinari commented Oct 29, 2019

@dimitropoulos This would be great! I have kept a small list of implementations I have seen over the last few months: https://trello.com/c/hMM2R1uh/1893-alert-component.

@oliviertassinari oliviertassinari added the new feature New feature or request label Oct 29, 2019
@dimitropoulos
Copy link
Member Author

Great! Then I'll start scoping out the API and features we would want to target. Thankfully there's a good deal to draw on! Once we agree on the general approach for things I will get started on the code.

@oliviertassinari
Copy link
Member

@oliviertassinari oliviertassinari changed the title [Alert] Alert component [Alert] Add new component Dec 1, 2019
@r3dm1ke
Copy link
Contributor

r3dm1ke commented Dec 5, 2019

Hi, I just wanted to ask if @dimitropoulos are you still working on it, or I could give it a go?

@oliviertassinari
Copy link
Member

Note that the banner component in the Material Design specification is very close to this use case, I think that it's more important to have a solid Alert, but it could be interested to have some sort of an extended support for this component.

@dimitropoulos
Copy link
Member Author

I have some work I did on it, but never pushed it up. @r3dm1ke would you like to work on it together? I'll push what I had now. It took me quite a long time to figure out just how to get the basic moving pieces into the codebase to have a place to start.

@r3dm1ke
Copy link
Contributor

r3dm1ke commented Dec 6, 2019

@dimitropoulos That would be awesome, I am just not sure how to work together on the same pull request at the same time. Perhaps we need to split it into two based on what you have already completed (like api/tests/docs etc.)? @oliviertassinari do you have any insights?

@dimitropoulos
Copy link
Member Author

see: #18702

I basically did a lot of the gruntwork associated with just getting the minimal set of things included. I'd be more than happy for you to pick it up and do whatever you wish from here if you'd be alright with me chipping in a few commits as you go. It was much more work than I was anticipating to get the above commit just because of a lack of familiarity with the codebase. That said, I'd really like to get to a point where I can contribute valuable features (such as this) to this project, so if you could lead the way, it would give me a better idea of how development on this project works in practice.

Either way: I'm totally cool with whatever. :)

@dimitropoulos
Copy link
Member Author

Also, you're in Toronto and I'm in Michigan, so perhaps our timezones line up well enough to do some pair programming on it some night if you want :)

@r3dm1ke
Copy link
Contributor

r3dm1ke commented Dec 6, 2019

see: #18702

I basically did a lot of the gruntwork associated with just getting the minimal set of things included. I'd be more than happy for you to pick it up and do whatever you wish from here if you'd be alright with me chipping in a few commits as you go. It was much more work than I was anticipating to get the above commit just because of a lack of familiarity with the codebase. That said, I'd really like to get to a point where I can contribute valuable features (such as this) to this project, so if you could lead the way, it would give me a better idea of how development on this project works in practice.

Either way: I'm totally cool with whatever. :)

I am actually not that experienced myself 😅. I have looked through your changes, could you give we write access to your fork of the repo so we can continue from there?

@dimitropoulos
Copy link
Member Author

oh my apologies! I assumed that you could already write to it! I'll do that real quick :)

@dimitropoulos
Copy link
Member Author

I've just sent both of you invites. I thought the way it worked was that if you have write access on the main repo that you would have access on the fork, but I think that was skewed by my usage of github within a private organization where this holds true.

@r3dm1ke
Copy link
Contributor

r3dm1ke commented Dec 6, 2019

I've just sent both of you invites. I thought the way it worked was that if you have write access on the main repo that you would have access on the fork, but I think that was skewed by my usage of github within a private organization where this holds true.

Thank you! I did not have write access to the main repo though. Could you send me your email address so we do not clutter this issue?

@oliviertassinari
Copy link
Member

oliviertassinari commented Dec 6, 2019

@dimitropoulos Thanks for the invitation, I think that the simplest would be to iterate on a single place: dimitropoulos:alert-component. It's already great to see the infrastructure work required to implement such Alert components done :). I can imagine that it was challenging, well done!

I will have a look at the benchmarked components and make a proposal (from a "product" perspective), it's great to have multiple 👀 challenging it. Once we agree on the deliverable, you guys should be able to move forward and see the next steps more clearly :).

@mbrookes
Copy link
Member

mbrookes commented Dec 6, 2019

It's already great to see the infrastructure work required to implement such Alert components done

I've wondered occasionally if we should automate it, but then convince myself that there can only be so many more new components. Now I'm not so sure! :)

@oliviertassinari
Copy link
Member

oliviertassinari commented Dec 6, 2019

  • I would propose the following API, something close to the DOM nodes like we have with the list, it makes composition and style override easier:
<Alert>
  <AlertIcon />
  <AlertTitle>
    Sorry, results couldn't be loaded
  </AlertTitle>
  <AlertClose />
  <AlertDescription>
    There was an error while processing your request. Refresh your page for loading results.
  </AlertDescription>
  <AlertActions>
    <Button>
      Refresh
    </Button>
    <Button component="a" href="mailto:[email protected]">
      Contact support
    </Button>
  </AlertActions>
</Alert>

Capture d’écran 2019-12-07 à 00 08 40
https://orbit.kiwi/components/alert-message/

  • We might be able to use CSS grid for positioning the elements, the challenge is to make sure it's supported with the older version of the spec IE 11 implements.
  • It's probably a good opportunity to introduce the success, info, and danger colors in the theme's palette.
  • I would propose we implement the 4 different states with a color prop.
  • We might be able to have a banner variant: https://material.io/components/banners/#.
  • I'm wondering if we shouldn't support these 3 variants https://dribbble.com/shots/7874860-Alerts-UI-design/attachments/486141?mode=media, with the one labeled as "Secondary Alerts", the second one as the default (we could call it text), have outlined and filled (the first one)

@mbrookes
Copy link
Member

mbrookes commented Dec 7, 2019

It's probably a good opportunity to introduce the success, info, and danger colors in the theme's palette.

👍 🎉 🚀

@r3dm1ke
Copy link
Contributor

r3dm1ke commented Dec 7, 2019

  <AlertClose />

I was wondering should we allow multiple close icons and an ability to override its appearance and/or behavior? There may be a use case when user can either close or snooze the alert. But that may belong in <AlertActions />. What do you think?

@oliviertassinari
Copy link
Member

oliviertassinari commented Dec 7, 2019

@r3dm1ke Without more information, I don't think that we should allow more than one close icon. I think that developers should be able to customize the look of the icon button, or not even render it if needed. Yeah, a snooze button in AlertActions sounds like a valid use case.

@oliviertassinari
Copy link
Member

I think that @mbrookes has made a brilliant connection at #18750 (comment).
What if we were enabling people to use the Snackbar to render the Alert (as a possible use case)?

We have a customization example with the Snackbar that we could actually make the filled variant of the Alert (it almost has the same design as previously proposed).

@dimitropoulos
Copy link
Member Author

dimitropoulos commented Dec 9, 2019

@oliviertassinari my gut reaction is that this could be great: but on thinking about it for a second I'm less confident because the actual UI behavior of a snackbar (also called toast) is so different (it's fixed position, dismissable with an animation, etc.). That said... I'm not deeply aware of how this project is structured.

Just to say it out loud: it seems to me that it's actually the Alert component that's the parent component. I understand it's strange to think about it this way because the Snackbar has been implemented first, but consider that the Snackbar is actually an extension of the functionality of the Alert in a lot of ways. Perhaps part of the work for creating Alert is also to change Snackbar to use the Alert implementation under the hood. This is good because it could mean simply refactoring a lot of that Snackbar code into Alert and consuming it while also freeing up Alert usecases as described in this issue.

@r3dm1ke
Copy link
Contributor

r3dm1ke commented Dec 10, 2019

@dimitropoulos @oliviertassinari I actually think that <Snackbar><Alert>...</Alert></Snackbar> makes a lot of sense. Alert component provides styling to display messages of various natures (the success, info, danger etc) and it can appear, well, pretty much everywhere. For example, at the top of the screen, in a form, in a chat, in a widget of a dashboard, and at the edge of the sceen as well. So it would probably be perfect for people that want the Alert appearance but Snackbar behavior to use them together. But I do not think we should build any positioning constraints into the Alert component so as to allow users to render where they please. What do you guys think?

@dimitropoulos
Copy link
Member Author

Yeah, that sounds good to me. As far as the API is concerned I'm open minded but not sure. Consider how the Avatar component, based pretty much entirely on the Chip component, doesn't exactly (via its API) call attention to the fact that it uses Chip under the hood. It basically just feels like a bespoke component. Perhaps we should do the same with Snackbar. At the same time... It could be cool to isolate that Snackbar behavior and give the user the ability to put whatever they want in a Snackbar, kinda like how a Modal/Dialog works.

@oliviertassinari
Copy link
Member

oliviertassinari commented Dec 10, 2019

From my perspective, the main purpose of the Snackbar component is to handle the complexity of displaying toasts on the screen. So it includes position, animation, stacking, timeout, imperative API. It could come unstyled.
I very much agree with the presentation draw by @r3dm1ke.

@dimitropoulos I don't understand your point regarding the Chip and the Avatar.

@dimitropoulos
Copy link
Member Author

@oliviertassinari I think I misunderstood how to use the Snackbar. I thought it was a ready-made component that comes with styles and all that.

My point was that when you use Avatar you don't do: <Avatar><Chip {...someOptions /></Avatar>, rather, you instead just do <Avatar {...someOptions} />. The UX of the API is such that you don't have to be even aware that under the hood, Avatar is really just a nice wrapper for Chip.

Similarly, we may want to do the same here. Instead of doing <Snackbar><Alert {...someOptions}</Snackbar> we may be better off with an API that looks like <Snackbar {...someOptions} />. But... now that I see that Snackbar doesn't work that way, I'll just default to whatever you all think is best.

@oliviertassinari
Copy link
Member

OK, thanks for the clarification. But notice that the Avatar isn't a wrapper for the Chip component. However, you can provide an Avatar to the Chip component. I think that it would be great to keep the API declarative, close to the underlying DOM node.

@dimitropoulos
Copy link
Member Author

doh

Well now.. not sure how I got that impression. Sorry for the confusion :|

@oliviertassinari
Copy link
Member

No problem :)

@r3dm1ke
Copy link
Contributor

r3dm1ke commented Dec 11, 2019

I drafted out a proposed API for the root Alert component, tell me what you think:

  • color: 'default' | 'primary' | 'secondary' | 'error' - color of the alert, pulling from the theme
  • variant: 'text' | 'outlined' | 'filled' | 'banner' - variants from here and here
  • raised: bool - not sure about this one, but we could provide a styling similar to that in Card
  • size: 'small' | 'medium' | 'large' - control the margin within the Alert simular to the size prop in Table
  • component: elementType - component to use to render the Alert
  • classes, className, children

I am not sure though, should we have an open prop, like one in Dialog? Because both of them have a Close component, so should we add an ability to control the visibility of the alert with a prop?

@dimitropoulos
Copy link
Member Author

I would really like if we saw the addition of success, info, danger or the like for this. Perhaps we start small with the initial feature and just allow it to be a very restricted set of colors (of course people can always override in the meantime) and then spin off some separate issues/work for introducing the other colors to the palette.

I, for one (and maybe for all), have had quite a few times where I've wished the palette had a danger color or an info color.

@r3dm1ke
Copy link
Contributor

r3dm1ke commented Dec 11, 2019

I fully agree, but that big of a feature (add new colors to the theme palette and update the color prop on all components that use it) probably belongs to a separate issue. @oliviertassinari how do we proceed on this?

@dimitropoulos
Copy link
Member Author

then spin off some separate issues/work for introducing the other colors to the palette.

Yep! that's what I'm trying to say. Start with as few colors as possible for Alert at first (maybe literally only one?) and then add the other colors when the palette improvements land.

@mbrookes
Copy link
Member

@r3dm1ke I think there's a consensus that we can add these static colours to the theme ahead of any dynamic theme improvements (#18094 (comment) and elsewhere).

If they are added as part of this PR, then the work to add them to other components (where appropriate) can be handled separately, and probably incrementally.

@oliviertassinari
Copy link
Member

@r3dm1ke

  • +1 for adding success, info, and warning.
  • I would encourage to wait for a compelling use case before adding the component prop.
  • I'm not sure about the size prop. I would encourage to wait for a compelling use case too.
  • By raised, is this a reference to the elevation prop of the Paper component? It sounds good, I think that we can base the Alert component on the Paper (so no need for raised, we would get elevation)

@r3dm1ke
Copy link
Contributor

r3dm1ke commented Dec 12, 2019

@oliviertassinari @dimitropoulos
raised was a reference to the prop with the same name of the Card component, and I think it just supplies elevation to the Paper under the hood, so they are interchangeable. It indeed makes sense to use Paper to base the Alert on, so it would not be hard to implement the elevation prop.

I just pushed a commit that adds warning, info and success colors to the palette, tell me what you guys think. I am no designer, so the specific colors I chose may not be ideal 😅, but it should be easy to change it.

@dimitropoulos
Copy link
Member Author

I think it would be best to do the changes to the palette in another, separate, PR with a separate issue. I was trying to say before that it might be best to implement Alerts (for now) with just one color (primary?) or maybe two (primary + secondary?) to keep it as simple as possible while we nail down the main functionality. I imagine there are other concerns and such that will come up when expanding the palette and therefore it seems prudent to me for other people to be able to weigh in on that feature as it stands.

Great head start on the palette stuff, though!

@oliviertassinari
Copy link
Member

I think it would be best to do the changes to the palette in another, separate, PR with a separate issue.

@dimitropoulos This sounds safer 👍.

@r3dm1ke
Copy link
Contributor

r3dm1ke commented Dec 13, 2019

Ok, sounds reasonable, I just misunderstood. Then I will revert the last commit and save it for later

@dimitropoulos
Copy link
Member Author

dimitropoulos commented Dec 13, 2019

@r3dm1ke (and anyone else interested) the conversation about theme palette colors is happening in #13875 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: alert This is the name of the generic UI component, not the React module! new feature New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants