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

Feature/notify #205

Merged
merged 16 commits into from
Oct 8, 2017
Merged

Feature/notify #205

merged 16 commits into from
Oct 8, 2017

Conversation

ibelar
Copy link
Contributor

@ibelar ibelar commented Jul 27, 2017

Notification

notify.js as an AtkPlugin

Add js plugin for creating nice notification.

  • Notification is attach to body using $.atkNotify(options)
  • Can be attach to any dom element using $('selector').atkNotify(options)
  • Can be personalize with:
    • type: success, error, info or warning,
    • icon: any of semantic-ui icon,
    • size: any of semantic-ui size (height) like mini, small etc. ,
    • transition for opening or closing: any of semantic-ui transition,
    • width in percentage,
    • position: topLeft, topCenter, topRight, bottomLeft, bottomCenter, bottomRight or center,
    • content: text for notification,
    • duration: time in ms.

notify.php

For demonstration purpose in demos.

romaninsh
romaninsh previously approved these changes Aug 22, 2017
Copy link
Member

@romaninsh romaninsh left a comment

Choose a reason for hiding this comment

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

love this!

Once merged, we can make CRUD use them.

@romaninsh
Copy link
Member

Few notes after I reviewed.

First it looks like a visual glitch is introduced:

screen shot 2017-08-25 at 14 14 01

Secondly unlike many other components there are no "simple usage". Perhaps something like this:

$button->onClick(function() { 
   return new jsNotify('All went well');
});

We need to decide what is the best positioning for this. Also a few question about the content - can anything be in there?

My final note is that the HTML escaping is not working, we should by default escape everything.

@romaninsh romaninsh dismissed their stale review August 25, 2017 13:17

Didn't test!

Copy link
Member

@romaninsh romaninsh left a comment

Choose a reason for hiding this comment

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

see my other comment.

@romaninsh
Copy link
Member

romaninsh commented Aug 25, 2017

That UI layouting problem is not only in your branch, I'll have to check why it's like that, do you also see it locally? looks like some element wasn't closed.

@ibelar
Copy link
Contributor Author

ibelar commented Aug 25, 2017

Humm, don't have the layout problem on my side.

@romaninsh
Copy link
Member

weird, ok, it's off the list anyway. Will have to investigate.

@romaninsh
Copy link
Member

will do some integration testing but on course for 1.2

@romaninsh romaninsh merged commit dee8e1c into atk4:develop Oct 8, 2017
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.

2 participants