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.

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

slim down donut and bar plots panels #491

Merged
merged 6 commits into from
Sep 13, 2023
Merged

Conversation

moontrip
Copy link
Contributor

@moontrip moontrip commented Sep 11, 2023

This will address #426. Detailed descriptions are listed in the following:

  • requirements (taken from the ticket)

  • remove Binning method radio buttons for variable types that don't need it (presumably categorical)
    : addressed

  • narrow the radio buttons for when they are needed - the default featured variable could be continuous
    : I think that one of the best approach is to use acronym for certain radio button label: there are not much room for reducing the spacing in the radio buttons, I suppose. I chose deviation as dev. as it is commonly used as a short word: or we may use Std as the acronym of the standard deviation. Just in case, full word is explained at button title, but perhaps a text below the radio buttons can be utilized as well.

  • Remove the "Donut marker controls" heading? It doesn't save any width but it does save height and is not telling the user anything useful
    : addressed

  • Think about long variable names in the variable picker (max width/ellipsis if not already implemented?)

  • Think about long value names (in the categorical config table) (ellipsis or wrap?)
    : for both cases, there were no implementation on the ellipsis for both variable picker and data table. After seeking out a solution, perhaps the easiest solution is to set max width, then lengthy strings are naturally wrapped

In addition, barplot width is also adjusted so that it can be fitted to the max width. And some margins are changed so that it can be better displayed overall.

Screenshots are attached in the following for demonstrating above changes:

  • original layout
    sam donut layout01

  • after changing: categorical variable with lengthy table values
    sam donut layout01-2

  • after changing: continuous variable (with lengthy variable at the variable picker). As mentioned above, (Dev. = Deviation) may be located in the bottom of the radio buttons
    SAM donut layout02

@moontrip moontrip requested a review from bobular September 11, 2023 18:12
@moontrip moontrip linked an issue Sep 11, 2023 that may be closed by this pull request
5 tasks
Copy link
Member

@bobular bobular left a comment

Choose a reason for hiding this comment

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

Looks great - thanks very much.

I have one trivial change to request and a comment about the table width for the Sample.species variable in the megastudy (qa.vectorbase.org back end at the moment)

I'm getting a horizontal scroll bar (see below).

image

Not the end of the world. One of the issues is that the heading "Distribution" is quite long. But there's no way to avoid that.

I can get a reasonable compromise by adding this CSS rule manually in dev tools for the content (TD elements).

.MesaComponent td {
	word-break: break-word;
}

This allows the long species name words (e.g. "quadrimaculatus") to be split over lines, but unfortunately without a hyphen.

image

Oh, wait, there's hyphens: auto

.MesaComponent td {
	word-break: break-word;
	hyphens: auto;
}

image

Hmm, but that's breaking too aggressively now. "al-bopictus" and "com-plex" are not necessary.

It looks a bit better without the word-break: break-word but the ideal solution would be to break when only absolutely necessary (e.g. like with word-break: break-word) and to use hyphens. Is that possible?

If not, I'd go with word-break: break-word; hyphens: auto; if that's easy to pass to Mesa as a custom TD style?

<RadioButtonGroup
containerStyles={
{
// marginTop: 20,
}
}
label="Binning method"
label="Binning method (Dev. = Deviation)"
Copy link
Member

Choose a reason for hiding this comment

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

Not too keen on the (Dev. = Deviation) in the header

I think Std. dev. would be perfectly understandable by anyone who already understands "Quantile".

Ideally mouse-overs would clarify but it's not worth implementing just for this. Maybe leave a comment for the future that we should add this if/when we implement mouseovers in RadioButtonGroup?

@bobular
Copy link
Member

bobular commented Sep 13, 2023

If not, I'd go with word-break: break-word; hyphens: auto; if that's easy to pass to Mesa as a custom TD style?

And if that's not possible, maybe change the final column heading to "Distrib."? Or do some fancy CSS to make it shorten to Distrib. when necessary (or ellipses or whatever).

@moontrip
Copy link
Contributor Author

@bobular Thanks again for your detailed review/observations/suggestions. In view of release date tomorrow, the simplest approach is: a) use Std. Dev.; b) use Distr. instead of Distribution which is quite simple as the name/label is defined in the frontend.

That being said, one of my concern to merge this PR is that this work is only for Donut config panel, not thinking of the other panel, Bar plots (chart marker) panel which has quite wide width: Bubble one is okay. Perhaps just moving Marker Y-axis control to the next line of Marker Y-axis control would work as a simple solution?

What do you think?

@bobular
Copy link
Member

bobular commented Sep 13, 2023

What do you think?

Hi. "Distrib." should be OK (not "Distr.") - and "Std. dev." note the lowercase "d" consistent with casing of "Equal interval" will be good.

I think your suggestions for bar markers (good catch!) would be good - wrap the (presumably flex) marker axis controls to the next line and do the same changes to the radio group (and DRY up if time permits).

@bobular
Copy link
Member

bobular commented Sep 13, 2023

Or maybe "Chart".

image

It's probably not trivial to make the count heading span both columns? That would also work.

@moontrip moontrip changed the title slim down donut marker panel slim down donut and bar plots panels Sep 13, 2023
@moontrip moontrip marked this pull request as ready for review September 13, 2023 18:34
@moontrip
Copy link
Contributor Author

@bobular I have addressed your feedbacks and suggestions including the chat we discussed at Slack. Also, bar plots panel is adjusted to be similar to donut panel. Here are screenshots of bar plots panel - captured using vectorbase mega study one. Feel free to let me know if you need something to change.

SAM-Chart-panel02

SAM-Chart-panel020

@moontrip
Copy link
Contributor Author

For record, I scaled down the preview marker from 3 to 2.5 which is better I suppose. Also added some spacing between the title and the preview marker. Thanks @bobular for your reviews and great suggestions 👍 Will merge this PR.

  • scale 3 (previous)
    SAM-Chart-panel022

  • scale 2.5 (changed)
    SAM-Chart-panel021

@moontrip moontrip merged commit 75ff58d into main Sep 13, 2023
1 check passed
@dmfalke dmfalke deleted the 426-donut-marker-config-panel branch November 28, 2023 18:25
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.

Map - slim down Donut marker config panel to a reasonable width
2 participants