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

Fixes #18426 - Add actions to notification entries #4265

Merged
merged 1 commit into from
Feb 23, 2017

Conversation

dLobatog
Copy link
Member

@dLobatog dLobatog commented Feb 9, 2017

This adds support for the notifications to display actions. In case no
actions are provided, the dropdown kebab isn't shown. The actions
possible right now only include GET links.

screenshot from 2017-02-09 12-16-25
screenshot from 2017-02-09 12-16-44

it('does not display actions dropdown if links are NOT provided', () => {
let notificationWithoutAction = notification
delete notificationWithoutAction['actions']['links'];
const dropdownMenu = setup(notificationWithoutAction).find('a');

Choose a reason for hiding this comment

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

Expected blank line after variable declarations newline-after-var

});
it('does not display actions dropdown if links are NOT provided', () => {
let notificationWithoutAction = notification
delete notificationWithoutAction['actions']['links'];

Choose a reason for hiding this comment

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

["actions"] is better written in dot notation dot-notation
["links"] is better written in dot notation dot-notation

expect(dropdownMenu.text()).toBe('Foreman blog');
});
it('does not display actions dropdown if links are NOT provided', () => {
let notificationWithoutAction = notification

Choose a reason for hiding this comment

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

Expected blank line after variable declarations newline-after-var
Missing semicolon semi

expect(dateElement.text()).toBe('12/13/16');
});
it('display actions dropdown if links are provided', () => {
const dropdownMenu = wrapper.find('a');

Choose a reason for hiding this comment

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

Expected blank line after variable declarations newline-after-var

'links': [
{
'href':'https://theforeman.org',
'title':'Foreman blog'

Choose a reason for hiding this comment

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

Missing space before value for key 'title' key-spacing

actions: {
'links': [
{
'href':'https://theforeman.org',

Choose a reason for hiding this comment

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

Missing space before value for key 'href' key-spacing

{actions["links"][i]["title"]}</a>
</li>
));
};

Choose a reason for hiding this comment

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

Unnecessary semicolon no-extra-semi

let li_key ="notification-link-" + i + "-" + id
links.push((<li key={li_key} id={li_key}>
<a target="_blank" href={actions["links"][i]["href"]}>
{actions["links"][i]["title"]}</a>

Choose a reason for hiding this comment

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

["links"] is better written in dot notation dot-notation
Strings must use singlequote quotes
["title"] is better written in dot notation dot-notation

for (let i = 0; i < actions["links"].length; i++) {
let li_key ="notification-link-" + i + "-" + id
links.push((<li key={li_key} id={li_key}>
<a target="_blank" href={actions["links"][i]["href"]}>

Choose a reason for hiding this comment

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

["links"] is better written in dot notation dot-notation
Strings must use singlequote quotes
["href"] is better written in dot notation dot-notation

let actions_dropdown = "";
if (actions["links"] != null) {
for (let i = 0; i < actions["links"].length; i++) {
let li_key ="notification-link-" + i + "-" + id

Choose a reason for hiding this comment

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

Expected blank line after variable declarations newline-after-var
Infix operators must be spaced space-infix-ops
Strings must use singlequote quotes
Missing semicolon semi


let actions_dropdown = "";
if (actions["links"] != null) {
for (let i = 0; i < actions["links"].length; i++) {

Choose a reason for hiding this comment

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

["links"] is better written in dot notation dot-notation
Strings must use singlequote quotes

const links = [];

let actions_dropdown = "";
if (actions["links"] != null) {

Choose a reason for hiding this comment

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

["links"] is better written in dot notation dot-notation
Strings must use singlequote quotes

@@ -18,6 +18,31 @@ const Notification = ({created_at, seen, text, level, id}) => {
(<strong {...tooltip} className="drawer-pf-notification-message pointer"
onClick={markAsRead}>{text}</strong>);

const links = [];

let actions_dropdown = "";

Choose a reason for hiding this comment

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

Expected blank line after variable declarations newline-after-var
Strings must use singlequote quotes

for (let i = 0; i < actions.links.length; i++) {
let li_key ='notification-link-' + i + '-' + id;
links.push((<li key={li_key} id={li_key}>
<a target='_blank' href={actions.links[i].href}>

Choose a reason for hiding this comment

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

Unexpected usage of singlequote jsx-quotes


if (actions.links != null) {
for (let i = 0; i < actions.links.length; i++) {
let li_key ='notification-link-' + i + '-' + id;

Choose a reason for hiding this comment

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

Expected blank line after variable declarations newline-after-var
Infix operators must be spaced space-infix-ops

@ohadlevy
Copy link
Member

ohadlevy commented Feb 9, 2017

@gailsteiger or @matanwerbner can you have a look please?

}

actions_dropdown = (
<div className="dropdown pull-right dropdown-kebab-pf">
Copy link
Member

Choose a reason for hiding this comment

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

does it make sense to make kebab a component by it self? i can see us using it in many different places

Copy link
Member

Choose a reason for hiding this comment

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

yes


actions_dropdown = (
<div className="dropdown pull-right dropdown-kebab-pf">
<button className="btn btn-link dropdown-toggle" type="button" id="dropdownKebabRight{id}"
Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

i think we can implement this component ourself using a stateful React class, more reason for it to be on it's own

Copy link
Member

@gailsteiger gailsteiger Feb 10, 2017

Choose a reason for hiding this comment

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

We should implement the kebab as a stand alone component using the patternfly markup as a reference. We probably should remove the bootstrap js functionality and replace with a pure react implementation.

Copy link
Contributor

Choose a reason for hiding this comment

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

thinking of it, the kebab is just an icon, so perhaps we can have it as a css class icon, with onClick to open various panels as component requires

Copy link
Member

@gailsteiger gailsteiger Feb 10, 2017

Choose a reason for hiding this comment

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

In patternfly, the kebab is a restyled version of a dropdown supporting the same features as the bootstrap dropdown component. I'll check with Roxanne as to whether there are use cases for the dropdown menu or whether the kebab should always be used.

const links = [];

let actions_dropdown = '';

Copy link
Contributor

@matanwerbner matanwerbner Feb 9, 2017

Choose a reason for hiding this comment

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

This component may best be put on it's own as a seperate component, accepting a list of links, and then on the parent's render:
{ actions.links && <ActionsDropdown links={actions.links } /> }
that way we dont inflate the notification component too much, and dont have to deal with different types of notifications

Copy link
Member

Choose a reason for hiding this comment

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

yes

let actions_dropdown = '';

if (actions.links != null) {
for (let i = 0; i < actions.links.length; i++) {
Copy link
Contributor

@matanwerbner matanwerbner Feb 9, 2017

Choose a reason for hiding this comment

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

it might be nicer to use array.map
`const links = actions.links.map((link, index) => {

})`

Copy link
Member

Choose a reason for hiding this comment

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

+1 for using map

@@ -27,6 +28,7 @@ const Notification = ({created_at, seen, text, level, id}) => {
return (
<div className="drawer-pf-notification">
<Icon type={level} css="pull-left"></Icon>
{actions.links && <Dropdown links={actions.links, id} />}

Choose a reason for hiding this comment

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

Unexpected use of comma operator no-sequences

<span className="fa fa-ellipsis-v"></span>
</button>
<ul className="dropdown-menu dropdown-menu-right" aria-labelledby="dropdownKebabRight{id}">
{list_links}

Choose a reason for hiding this comment

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

Identifier 'list_links' is not in camel case camelcase

links.map((link, i) => {
let li_key = 'notification-link-' + i + '-' + id;

list_links.push((<li key={li_key} id={li_key}>

Choose a reason for hiding this comment

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

Identifier 'list_links' is not in camel case camelcase
Identifier 'li_key' is not in camel case camelcase

const list_links = [];

links.map((link, i) => {
let li_key = 'notification-link-' + i + '-' + id;

Choose a reason for hiding this comment

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

Identifier 'li_key' is not in camel case camelcase

import React from 'react';

const Dropdown = ({ links, id }) => {
const list_links = [];

Choose a reason for hiding this comment

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

Identifier 'list_links' is not in camel case camelcase

@dLobatog
Copy link
Member Author

@ohadlevy @gailsteiger @matanwerbner

Updated - the Dropdown is now in its own component. It accepts the ID in order to create unique ids for the elements (which QE deems useful). I could see making it more general but I think it's best to leave it 'YAGNI' and add it when we actually want it.

Aside from that, I believe I addressed the other comments (using map instead of the for iterator, etc..)


const Dropdown = ({ links, id }) => {
const listLinks = [];

Copy link
Contributor

@matanwerbner matanwerbner Feb 13, 2017

Choose a reason for hiding this comment

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

functional approach to writing this would be:


const listLinks = links.map((link, i) => { 
  const liKey = `notification-link-${i}-${id}`
 return <li key={liKey} id={liKey}>
                    <a target="_blank" href={link.href}>
                    {link.title}</a>
                  </li>
}

Copy link
Member Author

Choose a reason for hiding this comment

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

That's true - haste is the enemy of doing a good job 😄 Will update in a moment

@@ -17,6 +17,7 @@ def payload
:subject => notification_blueprint.subject,
:created_at => notification.created_at,
:group => notification_blueprint.group,
:actions => notification_blueprint.actions
Copy link
Member

Choose a reason for hiding this comment

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

how does the actions translate to actual urls given the subject is part of the notification vs notification_blueprint object?

Copy link
Member Author

Choose a reason for hiding this comment

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

Actions have nothing to do with subject - it's just a dumb set of links for the moment, it can be any hash https://github.com/theforeman/foreman/pull/4265/files#diff-61072566998937ff274a277f94d284e8R12

It can be made more complicated but I'm not sure how good is it to start coding logic nothing is using, this approach works well with the RSS PR (if passing an array of links isn't good enough ,e.g: you want to pass the HTTP action - POST, etc.., that's easily changeable if we need to do it)


const Dropdown = ({ links, id }) => {
const listLinks = links.map((link, i) => {
const liKey = `notification-link-${i}-${id}`

Choose a reason for hiding this comment

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

Missing semicolon semi

<button className="btn btn-link dropdown-toggle" type="button" id="dropdownKebabRight{id}"
data-toggle="dropdown" aria-haspopup="true" aria-expanded="true">
<span className="fa fa-ellipsis-v"></span>
</button>
Copy link
Member

Choose a reason for hiding this comment

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

The component is still using bootstrap's dropdown.js plugin to handle the toggle. This is not desirable

Copy link
Member Author

@dLobatog dLobatog Feb 14, 2017

Choose a reason for hiding this comment

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

Why not? I see it's handled like that on Patternfly and the Angular implementation of Patternfly too.

Copy link
Member

Choose a reason for hiding this comment

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

Patternfly itself doesn't implement a javascript framework so it makes sense that it uses the bootstrap jquery plugins.

With Angular and React the paradigm moves away from direct manipulation markup and adopts the approach of rendering to reflect state. With React the concept of one way data flow is also introduced.

If you have a look at AngularUI Bootstrap (the port of Bootstrap to Angular) you can see that it contains native AngularJS directives and has no dependency on either jQuery or Bootstrap's JS. I don't know why Angular Patternfly chose to not to implement in the same manner.

I think that our components will be easier to reason about if we adopt the pure React approach instead of mixing Reach with the Bootstrap plugins. Specifically, the bootstrap approach was not workable with the Navigation Drawer and Alert/Toasts (which you can see when I update the PR).

I think would be better to encapsulate a click handler in the component rather than rely on the boostrap plugins. In that way it will be easier for us to manage state, render in the React way and initiate actions.

@matanwerbner what do you think?

Copy link
Contributor

Choose a reason for hiding this comment

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

Agree. We should try to not depend on bootstrap at all, especially on simple or trivial components. A bit of css wil get us where we need.

});

return (
<Dropdown className="pull-right dropdown-kebab-pf" pullRight key={id} id={`notifications-dropdown-${id}`}>

Choose a reason for hiding this comment

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

Line 16 exceeds the maximum line length of 100 max-len

@dLobatog
Copy link
Member Author

@matanwerbner @gailsteiger Pushed an updated version that uses the Dropdown component from react-bootstrap, it's quite useful.

I'll submit this implementation to https://idimaster.github.io/patternfly-react/ see if we can hopefully rely on this npm package for every Patternfly component 😄

});

return (
<Dropdown className="pull-right dropdown-kebab-pf" pullRight key={id}
Copy link
Contributor

@matanwerbner matanwerbner Feb 17, 2017

Choose a reason for hiding this comment

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

small comment on style, styling with flex box is easier then pull-right/pull-left... if the containing div has display:flex and justify-content:space-between, the elements drop into position nicely :)

Copy link
Member Author

Choose a reason for hiding this comment

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

I seem to not be able to do this properly as flex-box centers the text 😞

Copy link
Contributor

Choose a reason for hiding this comment

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

It's ok, it was just something i thought is the responsibility of the composer (wrapper) component to organize it's children - and not for the child to be opinionated about it's location.

i suggest reading up on flexbox anyway, you may find it easier to deal with then floats

Copy link
Member

Choose a reason for hiding this comment

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

@dLobatog you might find this useful A Complete Guide to Flexbox

@gailsteiger
Copy link
Member

@dLobatog I don't like react-bootstrap. I think it's overly complicated and tedious to use. I'm not going to object to the PR but I think a simple component would be better.

In this case, you had to use many element tags with the dropdown:

    <Dropdown className="pull-right dropdown-kebab-pf" pullRight key={id}
              id={`notifications-dropdown-${id}`}>
      <Dropdown.Toggle noCaret bsStyle="link">
        <Glyphicon bsClass="fa" glyph="ellipsis-v" />
      </Dropdown.Toggle>
      <Dropdown.Menu>
        {listLinks}
      </Dropdown.Menu>
    </Dropdown>

I would have to consult the documentation every time I used it.

Instead, I think we should be able to achieve something along the lines of

    <Dropdown type="kebab" key={id}  id={`notifications-dropdown-${id}`}>
        {listLinks}
    </Dropdown>

PS: I'm not opposed to using component libraries but some are better than others

@dLobatog
Copy link
Member Author

dLobatog commented Feb 20, 2017

@gailsteiger thanks for taking a look! About react-bootstrap, I thought it would be a good idea to use it since it is already a dependency of Foreman and the component is fully implemented there. This means different types of toggle, dividers, etc.. we don't have to write them. Additionally the docs are pretty good and with a large community I would expect it to not have many bugs or at least have them fixed at a faster rate than if I wrote our own foreman notifications dropdown component (we can always contribute back 😄 ).

@matanwerbner Is the flex-box CSS thing a deal-breaker? I'm no expert or anything but it's what I see the Patternfly implementors use in their examples (http://www.patternfly.org/pattern-library/communication/notification-drawer/#/_code) so I suppose they code Patternfly with the expectations pull-direction is used instead of flex-box.

@gailsteiger @matanwerbner ++ appreciate the feedback!

edit - react-bootstrap, not react-patternfly

@gailsteiger
Copy link
Member

gailsteiger commented Feb 20, 2017

@dLobatog I presume your above comments are about react-bootstrap and not about react-patternfly.
I am guilty of having introduced the react-bootstrap dependency into foreman. At present I think we use only one component - modal. I tried a few of the other components and removed them for various reasons.
For the purpose of this PR, leave the react-bootstrap Dropdown in.

I still would like to see the feature running. Could you refer me to a live site or help me get the appropriate data into my database.

@dLobatog
Copy link
Member Author

dLobatog commented Feb 23, 2017

@gailsteiger If you want to test this, you can create notifications this way:

1st - Run rake db:migrate to run the migration from this pull request that adds 'actions' to the Notifications table.
2nd - Open the Rails console with rake console from the Foreman directory
3rd - Issue this commands:

blueprint = NotificationBlueprint.new(:group => 'React devs',
                                      :message => 'Hi Gail! This is a notification message',
                                      :level => 'info',
                                      :expires_in => 10.months,
                                      :name => 'gailnotification',
                                      :actions => {"links"=>[{"href"=>"https://theforeman.org/blog", "title"=>"Link to blog"}]})

blueprint.save
notification = Notification.new(:initiator => User.first, :audience => Notification::AUDIENCE_GLOBAL, :notification_blueprint => blueprint)
notification.save

It should automatically show up on the UI after that.

@matanwerbner
Copy link
Contributor

@dLobatog seems good from my end,
I feel like the structure of the notification drawer can be greatly improved, but that is for another PR

@gailsteiger
Copy link
Member

@dLobatog thanks for the rails console instructions. Looks fine by me.

<Dropdown className="pull-right dropdown-kebab-pf" pullRight key={id}
id={`notifications-dropdown-${id}`}>
<Dropdown.Toggle noCaret bsStyle="link">
<Glyphicon bsClass="fa" glyph="ellipsis-v" />
Copy link
Member

Choose a reason for hiding this comment

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

why not use our Icon component?

Copy link
Member Author

Choose a reason for hiding this comment

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

I did not see any advantage on using that component vs using what's provided by react-bootstrap, is there any?

@ohadlevy
Copy link
Member

@matanwerbner can to explain regarding notification structure comment?

@ohadlevy
Copy link
Member

@dLobatog note that a test has failed, it does look like its un-related, but if you push again it would be nice to confirm.

This adds support for the notifications to display actions. In case no
actions are provided, the dropdown kebab isn't shown. The actions
possible right now only include GET links.
@dLobatog
Copy link
Member Author

Repushed to confirm the error some node tests in Travis is not this PR's fault

@ohadlevy ohadlevy merged commit db8b122 into theforeman:develop Feb 23, 2017
@ohadlevy
Copy link
Member

thanks @dLobatog, katello tests failures are unrelated to this pr.

@dLobatog dLobatog deleted the 18426 branch February 23, 2017 16:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants