-
-
Notifications
You must be signed in to change notification settings - Fork 189
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
feat(logs): user-selectable logs limit #1999
Conversation
When viewing debug logs, the current buffer limit of a 100 is full pretty quick. I'm adding an option for the user to choose a bigger limit (up to a 1000).
src/components/logs-page/index.tsx
Outdated
}} | ||
> | ||
{logLimits.map((limit) => ( | ||
<option key={limit} value={limit} selected={limit == store.getState().logsLimit}> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i guess the key
and value
are unnecessary and can be removed?
i put them there, as i kinda copy-pasted log-level
section ;)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please refactor store.getState().logsLimit out of this loop, somewhere to the top of render function, example:
const { logsLimit } = store.getState();
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
BTW, when I make a commit responding to a comment, I like to link the comment in the commit description (so that it gives more opportunity to discover why a particular change was made).
How do I link to this comment? ;-)
My commit message:
perf(logs): extracted var to optimize a loop
https://github.com/nurikk/zigbee2mqtt-frontend/pull/1999/commits/9bca1ad6688ad003b85faf0de4acb4c9e467e075#r1574572268
The result:
22:46:37.622: [zigbee2mqtt-frontend] git -c credential.helper= -c core.quotepath=false -c log.showSignature=false add --ignore-errors -A -f -- src/components/logs-page/index.tsx
22:46:37.630: [zigbee2mqtt-frontend] git -c credential.helper= -c core.quotepath=false -c log.showSignature=false commit -F /private/var/folders/ws/nzgf19n976gfpx235sryx8lc0000gn/T/git-commit-msg-.txt --
⧗ input: perf(logs): extracted var to optimize a loop
https://github.com/nurikk/zigbee2mqtt-frontend/pull/1999/commits/9bca1ad6688ad003b85faf0de4acb4c9e467e075#r1574572268
✖ footer's lines must not be longer than 100 characters [footer-max-line-length]
✖ found 1 problems, 0 warnings
ⓘ Get help: https://github.com/conventional-changelog/commitlint/#what-is-commitlint
husky - commit-msg hook exited with code 1 (error)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done 94b0f50
const limit = parseInt(e.target.value); | ||
store.setState({ | ||
logsLimit: limit, | ||
logs: [...store.getState().logs.slice(-limit)], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we should slice logs here, this will happen automatically once new log record arrives, and then handler can be simplified to
store.setState({logsLimit: parseInt(e.target.value)})
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this will happen automatically once new log record arrives
yep, but I think it's better for the user experience, if the slice is also done immediately upon selecting the option from the menu
it doesn't matter for 100 -> 1000 change, but it does in the case of changing 1000 -> 100 ;)
i don't think the extra slicing complicates this code so much, to make a worse UI instead…
Sidenote:
More in below PRs: |
When viewing debug logs, the current buffer limit of a 100 is full pretty quick.
I'm adding an option for the user to choose a bigger limit (up to a 1000).
Please feel free to correct me, as I've never used React before ;)