-
Notifications
You must be signed in to change notification settings - Fork 2.2k
Seer performance badges for attribute updates and render #720
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
Conversation
ibgreen
commented
Jun 19, 2017
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.
Very nice, but isn't the formatting of the ms badge strange? shouldn't we only see the number?
return formatted; | ||
} | ||
|
||
export function leftPad(string, length = 8) { |
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.
haha, better be safe ;)
import './experimental'; | ||
import './deprecated'; | ||
import './debug/seer-integration'; | ||
import './debug/seer-integration.spec'; |
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.
why changing this? we are already in the tests directory
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.
why changing this? we are already in the tests directory
Every other file in the test directory follows this .spec
convention.
This convention is pretty common in the JS community. FWIW, it does have the advantage of easily distinguishing the two files e.g. in sublime tabs or when do command-open in the Chrome debugger.
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.
sure
src/lib/layer-manager.js
Outdated
/** | ||
* Set an override on the specified property and update the layers | ||
*/ | ||
import {setPropOverrides, layerEditListener, initLayerInSeer, updateLayerInSeer} |
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.
what's this comment?
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.
what's this comment?
Yes looks strange, will remove.
Yes, we can play around with it, but I prefer to leave as-is for now. I want to see the count of how many updates have happened (often more important than the number). Also wanted the user to have a chance of understanding what it means. If we just have two numbers, it will also be confusing to the user. If I could provide a tooltip for these badges, with additional data might be the best option... |