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

Add a Twig component to display flash messages #6642

Open
wants to merge 4 commits into
base: 4.x
Choose a base branch
from

Conversation

javiereguiluz
Copy link
Collaborator

@javiereguiluz javiereguiluz commented Dec 12, 2024

We're going to transform the entire UI using Twig Components. We've already have 2 components and this adds the third one.

It's called Banner and it can render flash messages / notifications.

Some comments:

  • I called it Banner to follow GitHub's Primer component naming (https://primer.style/components/banner) I like GitHub components and I'm getting many ideas from them.
  • The prop names withDismissButton and variant are also inspired/copied from GtiHub components
  • I created an Enum (the first one in this bundle!; this is fine because we now require PHP 8.1 and enums are supported there) to define the possible values of variant. I opted for the namespace Config\Enum\... because we already have Config\Option\... for the constant-based classes. The idea is to turn (in the future) the constants into enums, so we need a different namespace. Do you like Enum\ or do you prefer a different name?

Thanks!

@javiereguiluz javiereguiluz added this to the 4.x milestone Dec 12, 2024
@dragosprotung
Copy link
Contributor

* I created an `Enum` (the first one in this bundle!; this is fine because we now require PHP 8.1 and enums re supported there) to define the possible values of `variant`. I opted for the namespace `Config\Enum\...` because we already have `Config\Option\...` for the constant-based classes. The idea is to turn (in the future) the constants into enums, so we need a different namespace. Do you like `Enum\` or do you prefer a different name?

Why not keep BannerVariant in the same namespace at the twig component ?

@javiereguiluz
Copy link
Collaborator Author

I don't want to use the exact same namespace because "component" != "component option".

But your comment gave me an idea: use the Config/Option namespace inside Twig/Component namespace

@dragosprotung
Copy link
Contributor

I don't want to use the exact same namespace because "component" != "component option".

But your comment gave me an idea: use the Config/Option namespace inside Twig/Component namespace

Yes, "component" != "component option", but will this option ever be used outside of configuring the component, or another component ?

@javiereguiluz
Copy link
Collaborator Author

Folks, thanks for the comments and reviews. I made most of the changes you proposed and also changed other things thanks to the comments shared on Symfony Slack. Thanks 🙌

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.

5 participants