-
Notifications
You must be signed in to change notification settings - Fork 48
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
Optimize dev views #3838
Optimize dev views #3838
Conversation
⛔ Feature branch deployment currently inactive.If the PR is still open, you can add the |
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.
as i understood from the code, you did the following:
- require authentication for the dev views
- reduce code duplication in controls
- use performance instead of Date to measure time in /performance
@@ -391,10 +122,115 @@ export default { | |||
selectValue: null, | |||
dateValue: '2020-01-01', | |||
timeValue: '2020-01-01T14:45:00+00:00', | |||
|
|||
headers: [ |
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 are the headers in data?
they don't change, don't they? -> they could be a computed value, which would be cached
but i don't know the best practice here.
https://stackoverflow.com/questions/50536254/vue-js-referencing-constants-in-data
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 is a config array for the datatable and is best practice. A computed value is only needed if it depends on other props/data and should be recomputed if at least one of the dependencies change. As you can see below for the items array (for instance the this.textfieldValue
in the id: 'text-field'
items).
direction: rtl; | ||
span { | ||
direction: ltr; |
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 looks strange, why do we need rtl, and then switch back to ltr in the inner span?
isn't this just right text alignment?
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 ensures that the column heading text is aligned to the right (just like the content), and the up/down arrow is on the left side.
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.
Rest looks good to me, thanks
What has been done in this PR:
require authentication for the dev views (it is needed anyway, because we don't have a writable endpoint without login)
reduce code duplication in controls (make manual testing more systematic)
use performance instead of Date to measure time in /performance (use browser api which is intended for performance measuring)
Next we should decide which endpoint we could use. (Some ideas: #3609 (comment))
This PR does not include the endpoint changes. This should be done in a next pr.