Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Double-line graph #13

Merged
merged 8 commits into from
Jan 30, 2021
Merged

Double-line graph #13

merged 8 commits into from
Jan 30, 2021

Conversation

jianhandev
Copy link
Collaborator

@jianhandev jianhandev commented Jan 28, 2021 •
edited

Screenshot 2021-01-29 at 5 07 04 PM

Other changes:

  • moved time series graph to separate page

@moziliar
Copy link
Owner

Very Cool! Can you make another y axis to annotate one of the two lines? (I think the current y axis annotates the change)

@jianhandev
Copy link
Collaborator Author

jianhandev commented Jan 28, 2021 •
edited

Very Cool! Can you make another y axis to annotate one of the two lines? (I think the current y axis annotates the change)

A y-axis on the right hand side right, I was thinking about that too

moziliar
moziliar requested changes Jan 28, 2021
View reviewed changes
src/main/java/teammates/storage/api/FeedbackResponseMonitorDb.java Outdated Show resolved Hide resolved
src/main/java/teammates/ui/webapi/CreateAccountAction.java Outdated Show resolved Hide resolved
src/web/app/components/response-time-series-chart/response-time-series-chart.component.html Outdated Show resolved Hide resolved
src/web/app/components/response-time-series-chart/response-time-series-chart.component.html Outdated Show resolved Hide resolved
src/web/app/components/response-time-series-chart/response-time-series-chart.component.ts Outdated Show resolved Hide resolved
src/web/app/components/response-time-series-chart/response-time-series-chart.component.ts Show resolved Hide resolved
src/web/app/components/response-time-series-chart/response-time-series-chart.component.ts Outdated Show resolved Hide resolved
src/web/app/components/response-time-series-chart/response-time-series-chart.component.ts Outdated Show resolved Hide resolved
@moziliar
Copy link
Owner

@jianhandev
Yes, otherwise the graph looks extremely confusing (what the y axis indicates? the rising graph or the fluctuating one? what do the data points mean?)

@daongochieu2810
Copy link
Collaborator

Add padding also please :)))

@moziliar
Copy link
Owner

moziliar commented Jan 30, 2021 •
edited

  • Spacing between the interval and duration, too cramped

  • The time display can show in 12h? A bit confusing looking at it now. If possible can let user choose? This is just an add-on, I think in general the target is achieved

UPDATE: I think the x axis is fine if you delete the :30 ones, coz they are easily inferable and is quite distracting

@moziliar moziliar merged commit 20b98a6 into moziliar:code-task-main Jan 30, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@moziliar moziliar moziliar requested changes

@daongochieu2810 daongochieu2810 daongochieu2810 left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
3 participants
@jianhandev @moziliar @daongochieu2810