[GitHub] incubator-nifi pull request: NIFI-280 take two

classic Classic list List threaded Threaded
9 messages Options
Reply | Threaded
Open this post in threaded view
|

[GitHub] incubator-nifi pull request: NIFI-280 take two

JPercivall
GitHub user danbress opened a pull request:

    https://github.com/apache/incubator-nifi/pull/14

    NIFI-280 take two

    Second try and NIFI-280's merge request.  Squashed my commits.  Hopefully this time works better

You can merge this pull request into a Git repository by running:

    $ git pull https://github.com/danbress/incubator-nifi NIFI-280-take2

Alternatively you can review and apply these changes as the patch at:

    https://github.com/apache/incubator-nifi/pull/14.patch

To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:

    This closes #14
   
----
commit 0b00aa18b051b14f29040cbfa94d8c04e4d82f81
Author: danbress <[hidden email]>
Date:   2015-01-31T14:02:23Z

    NIFI-280 - Adding new documentation framework component and incorporating
    in framework

commit f554f3d1b4a1561fae8b4828aa6c3688e2d8261b
Author: danbress <[hidden email]>
Date:   2015-01-31T17:29:07Z

    NIFI-280 rewording column headers and warning message

commit 30c652de4a54e3a00b2a6656bc457ccefa8c4a29
Author: danbress <[hidden email]>
Date:   2015-01-31T17:56:25Z

    NIFI-280 - Modifing existing existing documentation
    removing index.html and renaming to additionalDetails.html if there is relevent information

----


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|

RE: [GitHub] incubator-nifi pull request: NIFI-280 take two

Mark Payne
Dan,
Looked over this contribution. Excellent work here - will make all of our lives much easier! :)
Only thing I'm really unsure about here is when you are generating HTML, you are including the Apache header into the generated HTML page and I'm not sure that it should go in there.
If it were a document that went into our CM it would definitely need it. However, this is something that is getting auto-generated by NiFi at runtime, and it would be adding Apache License headers to documentation for components that are not part of Apache NiFi.
Can anyone confirm that I am correct here, that it doesn't need to be in the artifacts that NiFi generates at runtime or otherwise correct me if I'm wrong?
Thanks-Mark

> From: [hidden email]
> To: [hidden email]
> Subject: [GitHub] incubator-nifi pull request: NIFI-280 take two
> Date: Sat, 31 Jan 2015 18:01:43 +0000
>
> GitHub user danbress opened a pull request:
>
>     https://github.com/apache/incubator-nifi/pull/14
>
>     NIFI-280 take two
>
>     Second try and NIFI-280's merge request.  Squashed my commits.  Hopefully this time works better
>
> You can merge this pull request into a Git repository by running:
>
>     $ git pull https://github.com/danbress/incubator-nifi NIFI-280-take2
>
> Alternatively you can review and apply these changes as the patch at:
>
>     https://github.com/apache/incubator-nifi/pull/14.patch
>
> To close this pull request, make a commit to your master/trunk branch
> with (at least) the following in the commit message:
>
>     This closes #14
>    
> ----
> commit 0b00aa18b051b14f29040cbfa94d8c04e4d82f81
> Author: danbress <[hidden email]>
> Date:   2015-01-31T14:02:23Z
>
>     NIFI-280 - Adding new documentation framework component and incorporating
>     in framework
>
> commit f554f3d1b4a1561fae8b4828aa6c3688e2d8261b
> Author: danbress <[hidden email]>
> Date:   2015-01-31T17:29:07Z
>
>     NIFI-280 rewording column headers and warning message
>
> commit 30c652de4a54e3a00b2a6656bc457ccefa8c4a29
> Author: danbress <[hidden email]>
> Date:   2015-01-31T17:56:25Z
>
>     NIFI-280 - Modifing existing existing documentation
>     removing index.html and renaming to additionalDetails.html if there is relevent information
>
> ----
>
>
> ---
> If your project is set up for it, you can reply to this email and have your
> reply appear on GitHub as well. If your project does not have this feature
> enabled and wishes so, or if the feature is enabled but not working, please
> contact infrastructure at [hidden email] or file a JIRA ticket
> with INFRA.
> ---
     
Reply | Threaded
Open this post in threaded view
|

Re: [GitHub] incubator-nifi pull request: NIFI-280 take two

Joe Witt
Mark,

I believe you have it correct.  Headers are not required on generated
items.  They are required on all source files with 'few exceptions'[1].

As long as these HTML documents are being built during the build phase that
makes them not part of the source.  Remember the official release of apache
nifi are the source materials.  The resulting binaries are not the
releasable item as they are not 'open source'.

[1] http://www.apache.org/legal/src-headers.html#faq-exceptions

Anyone think I have this wrong?

Thanks
Joe

On Mon, Feb 2, 2015 at 2:33 PM, Mark Payne <[hidden email]> wrote:

> Dan,
> Looked over this contribution. Excellent work here - will make all of our
> lives much easier! :)
> Only thing I'm really unsure about here is when you are generating HTML,
> you are including the Apache header into the generated HTML page and I'm
> not sure that it should go in there.
> If it were a document that went into our CM it would definitely need it.
> However, this is something that is getting auto-generated by NiFi at
> runtime, and it would be adding Apache License headers to documentation for
> components that are not part of Apache NiFi.
> Can anyone confirm that I am correct here, that it doesn't need to be in
> the artifacts that NiFi generates at runtime or otherwise correct me if I'm
> wrong?
> Thanks-Mark
>
> > From: [hidden email]
> > To: [hidden email]
> > Subject: [GitHub] incubator-nifi pull request: NIFI-280 take two
> > Date: Sat, 31 Jan 2015 18:01:43 +0000
> >
> > GitHub user danbress opened a pull request:
> >
> >     https://github.com/apache/incubator-nifi/pull/14
> >
> >     NIFI-280 take two
> >
> >     Second try and NIFI-280's merge request.  Squashed my commits.
> Hopefully this time works better
> >
> > You can merge this pull request into a Git repository by running:
> >
> >     $ git pull https://github.com/danbress/incubator-nifi NIFI-280-take2
> >
> > Alternatively you can review and apply these changes as the patch at:
> >
> >     https://github.com/apache/incubator-nifi/pull/14.patch
> >
> > To close this pull request, make a commit to your master/trunk branch
> > with (at least) the following in the commit message:
> >
> >     This closes #14
> >
> > ----
> > commit 0b00aa18b051b14f29040cbfa94d8c04e4d82f81
> > Author: danbress <[hidden email]>
> > Date:   2015-01-31T14:02:23Z
> >
> >     NIFI-280 - Adding new documentation framework component and
> incorporating
> >     in framework
> >
> > commit f554f3d1b4a1561fae8b4828aa6c3688e2d8261b
> > Author: danbress <[hidden email]>
> > Date:   2015-01-31T17:29:07Z
> >
> >     NIFI-280 rewording column headers and warning message
> >
> > commit 30c652de4a54e3a00b2a6656bc457ccefa8c4a29
> > Author: danbress <[hidden email]>
> > Date:   2015-01-31T17:56:25Z
> >
> >     NIFI-280 - Modifing existing existing documentation
> >     removing index.html and renaming to additionalDetails.html if there
> is relevent information
> >
> > ----
> >
> >
> > ---
> > If your project is set up for it, you can reply to this email and have
> your
> > reply appear on GitHub as well. If your project does not have this
> feature
> > enabled and wishes so, or if the feature is enabled but not working,
> please
> > contact infrastructure at [hidden email] or file a JIRA
> ticket
> > with INFRA.
> > ---
>
>
Reply | Threaded
Open this post in threaded view
|

[GitHub] incubator-nifi pull request: NIFI-280 take two

JPercivall
In reply to this post by JPercivall
Github user danbress commented on the pull request:

    https://github.com/apache/incubator-nifi/pull/14#issuecomment-72547440
 
    Mark/Joe,
       Unless I hear anything to the contrary, I'll update the code to remove the license generation later tonight.
   
    Thanks!
   
    Dan


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|

[GitHub] incubator-nifi pull request: NIFI-280 take two

JPercivall
In reply to this post by JPercivall
Github user danbress commented on the pull request:

    https://github.com/apache/incubator-nifi/pull/14#issuecomment-72579336
 
    Just pulled out the license generation.  Let me know if there are any other problems.  Thanks!


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|

Re: [GitHub] incubator-nifi pull request: NIFI-280 take two

Mark Payne
In reply to this post by JPercivall
Dan,


I pulled this in and started looking around. It all appears to work well.


I’m a bit concerned about the table layout though for properties: it gets pretty cramped, and it looks like the two columns “Default” and “Values” are a single column: “Default Values”. Also, there’s no description that I can see for the Allowable Values.


I would suggest a bit of an alternate layout here: rather than having the Description in the table, having an “Info” icon that the user can hover over that provides a tooltip with the description, similar to how it’s done in the Processor Configuration dialog in the application. This frees up a lot of space in the table. Then, if there are descriptions for the Allowable Values, we can also have the same construct for getting the Description of the Allowable Value. We can also then expand the table headers to “Default Value,” “Allowable Values,” and “Expression Language” rather than using abbreviations.


What do you think of this approach?


The only other thing that I've noticed is in the  blurb about Properties, it has “any other” as a single word “Anyother.” 😊








From: danbress
Sent: ‎Monday‎, ‎February‎ ‎2‎, ‎2015 ‎9‎:‎10‎ ‎PM
To: [hidden email]





Github user danbress commented on the pull request:

    https://github.com/apache/incubator-nifi/pull/14#issuecomment-72579336
 
    Just pulled out the license generation.  Let me know if there are any other problems.  Thanks!


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|

Re: [GitHub] incubator-nifi pull request: NIFI-280 take two

Dan Bress
Mark,
  I'm on board with you.  I think the table layout can be made better with your suggestions.  I'll give your suggestions a shot and share the results later tonight.  Thanks!

  Another thing I noticed is that the "bold" for the required properties is not that bold.  I think the style should could be tweaked a little to improve this.

Dan Bress
Software Engineer
ONYX Consulting Services

________________________________________
From: Mark Payne <[hidden email]>
Sent: Tuesday, February 3, 2015 9:37 AM
To: [hidden email]
Subject: Re: [GitHub] incubator-nifi pull request: NIFI-280 take two

Dan,


I pulled this in and started looking around. It all appears to work well.


I’m a bit concerned about the table layout though for properties: it gets pretty cramped, and it looks like the two columns “Default” and “Values” are a single column: “Default Values”. Also, there’s no description that I can see for the Allowable Values.


I would suggest a bit of an alternate layout here: rather than having the Description in the table, having an “Info” icon that the user can hover over that provides a tooltip with the description, similar to how it’s done in the Processor Configuration dialog in the application. This frees up a lot of space in the table. Then, if there are descriptions for the Allowable Values, we can also have the same construct for getting the Description of the Allowable Value. We can also then expand the table headers to “Default Value,” “Allowable Values,” and “Expression Language” rather than using abbreviations.


What do you think of this approach?


The only other thing that I've noticed is in the  blurb about Properties, it has “any other” as a single word “Anyother.” 😊








From: danbress
Sent: ‎Monday‎, ‎February‎ ‎2‎, ‎2015 ‎9‎:‎10‎ ‎PM
To: [hidden email]





Github user danbress commented on the pull request:

    https://github.com/apache/incubator-nifi/pull/14#issuecomment-72579336

    Just pulled out the license generation.  Let me know if there are any other problems.  Thanks!


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|

Re: [GitHub] incubator-nifi pull request: NIFI-280 take two

Dan Bress
Mark,
   I tried what you suggested last night, and in my opinion, now the table is a little too sparse looking.  Maybe CSS magic can help, but right now I think it looks a little naked.  I don't have the tooltip over the "info" icon working yet.  But this gives you an idea of what it would look like.

   I tried another idea of rendering the description as a second row below the description/default values/valid values/expression language row.  I kinda like this, although it definitely needs CSS magic to make it not confusing.  Right now I just slammed it to the right to make it not get mixed up with the property name on the left.  I think playing with row colors or borders a little may make it clearer, I'll play with this later and let me know what you think.

   I've supplied three renderings.  How it was originally working[1], putting the description in a tool tip as Mark suggested[2], moving the description to a second row[3].  Let me know if what you like/don't like.

[1] http://danbress.github.io/generated-documentation/components-02032015a/org.apache.nifi.processors.standard.GetFTP/index.html
[2] http://danbress.github.io/generated-documentation/components-02032015b/org.apache.nifi.processors.standard.GetFTP/index.html
[3] http://danbress.github.io/generated-documentation/components-02032015c/org.apache.nifi.processors.standard.GetFTP/index.html

Dan Bress
Software Engineer
ONYX Consulting Services

________________________________________
From: Daniel Bress <[hidden email]>
Sent: Tuesday, February 3, 2015 4:44 PM
To: [hidden email]
Subject: Re: [GitHub] incubator-nifi pull request: NIFI-280 take two

Mark,
  I'm on board with you.  I think the table layout can be made better with your suggestions.  I'll give your suggestions a shot and share the results later tonight.  Thanks!

  Another thing I noticed is that the "bold" for the required properties is not that bold.  I think the style should could be tweaked a little to improve this.

Dan Bress
Software Engineer
ONYX Consulting Services

________________________________________
From: Mark Payne <[hidden email]>
Sent: Tuesday, February 3, 2015 9:37 AM
To: [hidden email]
Subject: Re: [GitHub] incubator-nifi pull request: NIFI-280 take two

Dan,


I pulled this in and started looking around. It all appears to work well.


I’m a bit concerned about the table layout though for properties: it gets pretty cramped, and it looks like the two columns “Default” and “Values” are a single column: “Default Values”. Also, there’s no description that I can see for the Allowable Values.


I would suggest a bit of an alternate layout here: rather than having the Description in the table, having an “Info” icon that the user can hover over that provides a tooltip with the description, similar to how it’s done in the Processor Configuration dialog in the application. This frees up a lot of space in the table. Then, if there are descriptions for the Allowable Values, we can also have the same construct for getting the Description of the Allowable Value. We can also then expand the table headers to “Default Value,” “Allowable Values,” and “Expression Language” rather than using abbreviations.


What do you think of this approach?


The only other thing that I've noticed is in the  blurb about Properties, it has “any other” as a single word “Anyother.” 😊








From: danbress
Sent: ‎Monday‎, ‎February‎ ‎2‎, ‎2015 ‎9‎:‎10‎ ‎PM
To: [hidden email]





Github user danbress commented on the pull request:

    https://github.com/apache/incubator-nifi/pull/14#issuecomment-72579336

    Just pulled out the license generation.  Let me know if there are any other problems.  Thanks!


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|

[GitHub] incubator-nifi pull request: NIFI-280 take two

JPercivall
In reply to this post by JPercivall
Github user danbress closed the pull request at:

    https://github.com/apache/incubator-nifi/pull/14


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---