Skip to content

Conversation

@zspitzer
Copy link

  • collapse long stack traces
  • wrap long lines
  • add search and reload to view log
  • renamed details to analysis
  • fixed crash viewing individual detail record (serializeJson instead of
    serialize)
  • added page titles
  • always include the plugin stylesheet for each template
  • fixed sortable header colors (clashed with bg)
  • bumped the version to 2.4.0

- collapse long stack traces
- wrap long lines
- add search and reload to view log
- renamed details to analysis
- fixed crash viewing individual detail record (serializeJson instead of
serialize)
- added page titles
- always include the plugin stylesheet for each template
- fixed sortable header colors (clashed with bg)
- getLog function
- extra styles
- also include context url in dropdown
the script tag is added using cfhtmlbody, so it's useful to be able to
see where it's coming from
Copy link
Owner

@paulklinkenberg paulklinkenberg left a comment

Choose a reason for hiding this comment

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

Hi Zac,

Thanks very much for your PR. Much appreciated!
I have some remarks on the PR. I hope you can see it as feedback, not negative comment on your work. I do appreciate it!
But since I am responsible for this project, I have to be thorough.
When you can change / check the following items, I would be more then happy to pull a new PR in.

Kind regards, Paul Klinkenberg

  • Please revert the changes of attribute "scope" to "class" in [th scope="row"] (see https://www.w3schools.com/tags/att_th_scope.asp if you did not know this attribute)
  • In language file, the German language has no [custom key="analysis"/]
  • In build.properties, please revert the Author line. I am very grateful for your contribution, and I will add a new line with you being a contributor. But a contribution does not make you an author.
  • Files in /dist/: please do not submit. I will create the build myself, because that is the only way I can make sure the binaries I send out are genuine.
  • Css: if you add abbr { text-decoration: none; }, how is anybody going to notice it is an abbr? If you would re-style it, okay, but just removing the styling does not seem to be the proper way.
  • Improvement in viewlog.cfm: use IDs instead of classnames when referring to buttons and input fields from jQuery (way faster!).
  • In viewlog.cfm, I assume the .on("submit",...) function on the input field does not work: onSubmit only exists for form elements afaik? Which would mean that pressing enter in that search box (cool new function btw), returns the user to the previous page. I could be wrong here btw.
  • I do not agree with breaking log lines into multiple lines of max. 150 characters. The main objective of a log viewer, is to view the actual log files. A discussion could be, whether or not to use pre-wrap in the log line output, but just limiting the display of a line to 150 characters is not a good solution in my opinion. You could add some styling which wraps long lines, maybe that is a good thing...
  • I do like the idea of giving the log items a max-height by default, and an expand button / link like you did.
    This could be accomplished by putting each log item into one div (instead of each line in a div, as I did previously), then setting a max-height to those divs, and adding the expand link if the #listLen(logItem, chr(10))# is greater then n (eg. n=10 lines, x 16 pix line height = 160 max-height)
  • Optional improvement: In function getLog of Action.cfc, the variable "num" is not doing anything useful. You could use "if (columns eq ''){ ... } else { ...}" and forget about the num variable.
  • When doing a pull request in general, please keep your submission clean. Remove any extraneous formatting changes you made to pieces of code which you did not edit. In this case, I see whitespace removal on empty lines, cfml tags on new lines where they previously appeared on the previous line, and removal / addition of empty lines, in places where no "real" edits have been done. This makes the actual changes harder to see, and it has no use to add this to the pr.
  • Just a FYI: instead of using two lines of code, where line 1 declares a "var" variable, and line 2 where the variable is used, you can also use the local scope to get there in one line: eg. [cfsavecontent variable="local.back"/]

@paulklinkenberg
Copy link
Owner

Hi @zspitzer, Just checking how this PR is going. Do you need any help?
Kind regards, Paul

@zspitzer
Copy link
Author

zspitzer commented Apr 24, 2018

Hi @paulklinkenberg, I kept going on a tangent https://github.com/zspitzer/lucee-loganalyzer/tree/refactoring and then got distracted with working on improving the lucee docs

the remaining things I wanted to finish off was a summary view, sort of like the old analysis
and integrating a date range picker

@paulklinkenberg
Copy link
Owner

paulklinkenberg commented Apr 24, 2018 via email

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