Skip to content

Alert tag#296

Open
pgregorova wants to merge 16 commits intodevelopfrom
tag-alert
Open

Alert tag#296
pgregorova wants to merge 16 commits intodevelopfrom
tag-alert

Conversation

@pgregorova
Copy link
Copy Markdown
Member

Yet another common tag or component we tend to use on each project, so wanted to add it in. :)

I just created some basic styles for success, warning, error and information messaging.

Copy link
Copy Markdown
Contributor

@bstaruk bstaruk left a comment

Choose a reason for hiding this comment

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

Looks good to me! We could craft some JS around this tag to make the alerts disappear after some time (like a toast notification, optional of course), or closable with an icon, but that doesn't have to be in this PR.

Comment thread source/elements/tags/Alert/Alert.jsx Outdated

return (
<Tag className={classStack} {...attrs}>
<div className="Alert-body">
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.

BEM stylings dictate this should be Alert__body instead of Alert-body. It also looks like you aren't adding any styles to this div. Is it necessary to include?

Comment thread source/elements/tags/Alert/README.md Outdated
@@ -0,0 +1,10 @@
# Alert

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.

Some documentation here would be great! Even if it's minimal! I think some explanation on the contexts of each variant would be very useful here.

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 have updated documentation and extended "global colors" within our variables. In addition, I have extended the alert type based on level it's being used, as it seems to be a common trend across projects (page/form and field-level type)

That was also partially the reason why I had that body inner container for alerts that may be page-level where the colored background extends to edges of your viewport but the messaging retains body copy width and is centered.

Additionally, I assumed we may have an example with close button as well.

Copy link
Copy Markdown
Contributor

@CSKingMartin CSKingMartin Dec 5, 2017

Choose a reason for hiding this comment

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

I think this conversation is important. I think what we want to add to FC are elements that are atomic enough we don't need to generalize them. You had good thoughts on including that internal container, and a close button. But because those are things we have to 'cut' to make this new tag general enough for FC, is that a component we want to include in our starting library? Thoughts @drolsen @elseloop @bstaruk ?

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.

With @bstaruk we’ve been debating hat maybe some of these tags don’t necesaeily need to merged into master, maybe just keep them in dev, or have a branch with our internal components we tend to reuse a lot on projects.

When it comes to alerts, I’ve been seeing them as one of the common tags in our pattern libraries, as almost every site out there has some form of notifications.

Let’s definiteky keep talking on what we should consider adding to our base and what we should archive some place for easy reuse upon needs.

Copy link
Copy Markdown
Member

@drolsen drolsen Dec 5, 2017

Choose a reason for hiding this comment

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

I'm seeing this as a "when needed" vs. baseline tag as well. However, I don't want us to lose these (and more) great pieces of work by not capturing them somewhere useful for us. I think it might be time for us to create a third branch called "library" that sits along develop and master.

I propose each dev will need to determine if what they are pushing is going to be for this new library branch or baseline develop from now on and PR accordingly.

We will continue our normal review process to ensure we all have a chance to agree upon any changes requested. As we start new projects, we simply keep library in our back pockets and pick what we need, when we need, and modify from there.

Thoughts?

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.

@drolsen I have pushed few more updates and improvements to this tag, including your svg fix #314.

export default [{
name: 'Default Alert',
component: (
<Alert>Lorem ipsum</Alert>
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.

It looks like you may have intended this example to be labeled 'Default Example' in the HTML, and the below example to read 'Success alert'

Comment thread source/elements/tags/Alert/Alert.css Outdated
@@ -0,0 +1,47 @@
.Alert {
background-color: #f1f1f1;
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.

It would be helpful to create color variables for these background-colors in the 'variables/colors.css' file. That way they are more easily targeted when a project gets up and running. They would fit well under the /* general colors */ comment.

Petra Gregorová added 13 commits December 5, 2017 10:00
…tyles.

- created general color variables that can be reused for commonly used tags or components.
…t link variant as well for easy reuse.

Still need to finish few more touches on alerts to make it more "complete"
…endering and applied background images to body, instead of parent container so the icon would retain positiining in relation to body, mainly seen on page-level messaging versus fomr-level.
@pgregorova
Copy link
Copy Markdown
Member Author

I've made bunch updates and improvements while making sure it also passes accessibility. All there is left to resolve the SVG issue, so all the icons would render as expected.

Then we can decide where we could add this component, within FC core or elsewhere for future use on new projects.

Petra Gregorová added 2 commits January 16, 2018 08:19
- made green color lighter and removed background image from field-level alerts
@pgregorova pgregorova removed the request for review from elseloop January 30, 2018 13:08
@pgregorova
Copy link
Copy Markdown
Member Author

pgregorova commented Jan 30, 2018

Just wanted to let you @bstaruk @CSKingMartin and @drolsen know that I have updated this branch with Atomic FC and moved Alert to be part of molecules, instead of atoms

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants