Add search bar and filters for logcat output #147
Conversation
|
Hey @navstyachka could you PTAL?) |
|
Hi @tokou! Thank you for the contribution, but as soon as this is a massive change, I will need more time to review that. |
|
I just noticed that it does not work when opening the index file directly in the browser. It works when you serve it from a local web server though. |
|
I made an update to get the logs from the DOM tree instead of reading the logcat file. |
|
The build seems to have failed for reasons unrelated to the code. Any way to retrigger it ? |
|
@tokou I've triggered it |
There was a problem hiding this comment.
Thank you very much for your contribution and the idea itself! But I am sorry to say that for the moment your code does not meet our code standards and this pull needs not just some fixes but a totally different code approach and cannot be merged. We will be happy to have a look at how to improve Composer features and especially the search. I hope my comments will help you to improve your React and JS overall knowledge!
| } | ||
|
|
||
| loadStaticLogs() { | ||
| let logDivs = document.querySelectorAll('#static_logs > div > div') |
There was a problem hiding this comment.
It's a bad practice to reach elements directly from DOM in react, because it is against the whole concept of virtual DOM, except for rare cases (like reaching out <head> tag and some other corner cases).
Also, just fyi, using operator > is a bad practice, and sticking to simple tag div (without id or data-attribute) is also a bad practise — if HTML changes, the code is broken.
| return | ||
| } | ||
| let logs = [] | ||
| for (var div of logDivs) { |
There was a problem hiding this comment.
We don't use var for defining variables, as we use es-2015 syntax. Please check it out here: https://old.babeljs.io/learn-es2015/#let-const
Also, you can use method map for array iterating. Please check it out here: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/map
| for (var div of logDivs) { | ||
| logs.push(Object.assign({level: div.className, line: div.innerText, key: logs.length})) | ||
| } | ||
| this.setState({logs: logs, results: logs, loading: false}) |
There was a problem hiding this comment.
Fyi, in es6, if a key is the same as a value, you can write it like this: {logs, results: logs, loading: false}, please check it out here: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/Object_initializer
| } | ||
|
|
||
| componentDidMount() { | ||
| document.addEventListener('DOMContentLoaded', (event) => { |
There was a problem hiding this comment.
It has no sense adding listener DOMContentLoaded as the method componentDidMount itself says that the DOM is ready. Please read the React documentation to understand React way: https://reactjs.org/docs/state-and-lifecycle.html
| } | ||
| let logs = [] | ||
| for (var div of logDivs) { | ||
| logs.push(Object.assign({level: div.className, line: div.innerText, key: logs.length})) |
There was a problem hiding this comment.
There's no sense in using Object.assign here — what you actually want is just to add a new object to the array. Object.assign is a method used to perform more complicated actions with the objects. Please see the documentation here: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Object/assign
Also, if you want to add a unique key, here comes the map method — it has a unique index for each item in the array.
| logs.push(Object.assign({level: div.className, line: div.innerText, key: logs.length})) | ||
| } | ||
| this.setState({logs: logs, results: logs, loading: false}) | ||
| window.document.getElementById("static_logs").remove() |
There was a problem hiding this comment.
Changing DOM directly with JS in React is a forbidden action and it is against basic React rules. Please read the React manual for more information.
|
|
||
| render() { | ||
| return this.state.hide | ||
| ? (<div></div>) |
There was a problem hiding this comment.
In React, if we don't need a component to render, we return null
| <div> | ||
| <div className="card"> | ||
| <div className="title-common">LOGS</div> | ||
| <LogFilterButton onClick={ () => this.performFilterSearch("level:verbose") } text="Verbose" disabled={ !!!this.props.data.length } /> |
There was a problem hiding this comment.
Using !!! is not a good practice — what you basically do is converting a number to a boolean with the !! and checking the opposite value. In JS you can use just number to check for true or false. If the number is 0 — it's a false value, if > 0 — it's a true value.
| <div className="card"> | ||
| <div className="title-common">LOGS</div> | ||
| <LogFilterButton onClick={ () => this.performFilterSearch("level:verbose") } text="Verbose" disabled={ !!!this.props.data.length } /> | ||
| <LogFilterButton onClick={ () => this.performFilterSearch("level:debug") } text="Debug" disabled={ !!!this.props.data.length } /> |
There was a problem hiding this comment.
If you use smth more than 1 time — put it into a variable.
|
|
||
| componentWillReceiveProps(props) { | ||
| let data = props.data; | ||
| if (data === this.state.data) { |
There was a problem hiding this comment.
This code will always result in false, please read the JS documentation on how arrays works.
|
Hi @navstyachka |
There are many ways to approach this.
I decided against adding to the
window.testobject as logcat output can be very large.I kept the static html generated by the Jar. I just added an id
static_logsin order to remove it once we load the logs in js.I made a
LogContainercomponent that shows the filters, the search bar and the logs once they are loaded from the file (it replaces the static logs).I extracted the
SearchBarcomponent and made it generic and used byLogFilterandSuiteFilterI tested this with a 10k lines logcat output file. Loading is seamless thanks to the html. Search is kinda laggy but acceptable. Maybe there's a React trick out there to make it faster.