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

Added Alert component #28

Merged
merged 4 commits into from
Nov 13, 2018
Merged

Conversation

guastallaigor
Copy link
Contributor

This address #27 .

Added in Docs:

1. Alert component:

image

2. Show documentation button:

image


If I'm missing something, or you want something to be different please just let me know. Thank you.

@LeCoupa
Copy link
Owner

LeCoupa commented Nov 9, 2018

Thank you. I'll merge this in a few hours. :)

I will probably make some changes also.

@LeCoupa LeCoupa self-requested a review November 10, 2018 18:59
@LeCoupa
Copy link
Owner

LeCoupa commented Nov 13, 2018

I made a few changes to your PR. Will merge tonight :)

@LeCoupa LeCoupa merged commit 013114d into LeCoupa:master Nov 13, 2018
@LeCoupa
Copy link
Owner

LeCoupa commented Nov 13, 2018

Among other things, here is what I did:

  • Transition should be added by the developer himself. It should not be part of the component.
  • I removed the title prop for the time being. I will add it later if I find a way to style it accordingly.
  • I used flexbox to style and adjust the content.
  • I added the "close" event so people can subscribe to it and close the banner by themselves (and other things if they need to).

https://www.vuedarkmode.com/#baseAlert

Thank you again for your work! :) If you disagree with some of my changes, feel free to tell me.

Also, can I add you as a contributor on the readme? (will add this part very soon)

@guastallaigor
Copy link
Contributor Author

guastallaigor commented Nov 13, 2018

@LeCoupa Thank you. The alert docs looks a lot cleaner and better now. I don't disagree with anything.

You can add me in the contributor on the readme if you want :).

Just a side note, maybe add it in the readme base components also?

@LeCoupa
Copy link
Owner

LeCoupa commented Nov 13, 2018

Great! Yes I added it to the readme :)

@LeCoupa
Copy link
Owner

LeCoupa commented Dec 5, 2018

Just added you to the contributors on the readme. :-)

@guastallaigor
Copy link
Contributor Author

Just added you to the contributors on the readme. :-)

Thank you :)

@scoutrul
Copy link

scoutrul commented Jun 5, 2020

Where is this future now??

@LeCoupa
Copy link
Owner

LeCoupa commented Jun 5, 2020

Hello @scoutrul, do you mean "this feature"? Or are you referring to something else?

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.

3 participants