Skip to content

feat: added button for global min/max toggle, fixes #74 #88

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

Merged
merged 9 commits into from
Aug 12, 2021

Conversation

AndyBuchwald
Copy link
Collaborator

No description provided.

@tilsche tilsche self-requested a review June 23, 2021 15:04
Copy link
Contributor

@tilsche tilsche left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just looked at the code so far.

index.html Outdated
@@ -33,6 +33,10 @@
<img class="navimg" src="img/icons/file-earmark-arrow-down.svg"/>
Export
</button>
<label class="btn btn-outline-primary">
<input type="checkbox" id='checkbox_min_max' checked=true v-on:click="toggleMinMaxButtonClicked"> min/avg/max
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since, avg is always shown with this button, we can remove it from the description text.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The glue between the checkbox element and the JS code should probably be v-model or v-bind. I'm just not sure how this can deal gracefully with the intermediate state (see vuejs/vue#4094).

getMetricDrawState (metricName) {
for (const metricBase in this.state.allMetrics) {
const metricArray = []
if (this.state.allMetrics[metricBase].name === metricName) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We really need a better way to get a metric by name other than looping through all metrics... But I guess this is a different refactoring issue.

@tilsche
Copy link
Contributor

tilsche commented Jun 24, 2021

The global toggle is very good already. However, we didn't really think about showing only min or mix without the other as we currently only have the min-to-max band. I think in the show_min != show_max configurations, we will just draw a line for each level that was selected.

@tilsche
Copy link
Contributor

tilsche commented Jun 24, 2021

Oddly, the global toggle field is not vertically aligned with the other global buttons in my browser (both chromium and firefox).

@AndyBuchwald
Copy link
Collaborator Author

The global toggle is very good already. However, we didn't really think about showing only min or mix without the other as we currently only have the min-to-max band. I think in the show_min != show_max configurations, we will just draw a line for each level that was selected.

Fixed

@AndyBuchwald
Copy link
Collaborator Author

Oddly, the global toggle field is not vertically aligned with the other global buttons in my browser (both chromium and firefox).

fixed

Copy link
Contributor

@tilsche tilsche left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please try to stick to the v-model if possible. Create concrete example where it doesn't work as expected (in both directions) and consult with @kinnarr .

@tilsche tilsche merged commit 3992ac9 into master Aug 12, 2021
@tilsche tilsche deleted the fix-issue-74 branch August 12, 2021 08:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants