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

report overhaul phase 3 #326

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from
Draft

Conversation

ayobi
Copy link
Contributor

@ayobi ayobi commented Dec 17, 2024

Added 'Your Five Most Abundant Microbes' boxplots and 'How to read a boxplot' modal for the composition tab.

Lacking:

  • Stats of microbe e.g. Median of Escherichia
  • 'Five Most Commonly Found Microbes in Our Dataset' boxplots
  • Tabs: 'Entire dataset | 100 samples most similar to yours'

@wasade
Copy link
Member

wasade commented Dec 17, 2024

Thanks! Could a rendered example be provided?

@ayobi
Copy link
Contributor Author

ayobi commented Dec 17, 2024

Sure thing @wasade, below are some screenshots, some zoomed out to show more
full screen zoomed out
normal size boxplots
zoomed out box plots
zoomed out modal

@wasade
Copy link
Member

wasade commented Dec 17, 2024

Thanks!

const q3 = relativeAbundance * 0.75;

return {
genus: d.Genus || d.genus || "Unknown",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
genus: d.Genus || d.genus || "Unknown",
genus: d.Genus || d.genus || "{{ _('Unknown') }}",

All strings that are/could be displayed to the user need to be exposed to the I18N mechanism. Only tagging this instance, but please review all of the code for this issue.

Comment on lines +652 to +653
console.log(`Data for genus relativ ${d.relativeAbundance}:`, d);
console.log(`Index: ${i}`);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
console.log(`Data for genus relativ ${d.relativeAbundance}:`, d);
console.log(`Index: ${i}`);

Comment on lines +623 to +644
const columns = 2;
const panelWidth = 600;
const panelHeight = 240;
const spacingX = 40;
const spacingY = 70;

const totalWidth = 1280;
const totalBoxWidth = columns * panelWidth + (columns - 1) * spacingX;
const remainingWidth = totalWidth - totalBoxWidth;

const rows = Math.ceil(mockData.length / columns);

const margin = { top: 20, right: 0, bottom: 60, left: -20 };

const totalHeight = rows * panelHeight + (rows - 1) * spacingY + margin.top + margin.bottom;

d3.select("#boxplot-svg").selectAll("svg").remove();

const svg = d3.select("#boxplot-svg")
.append("svg")
.attr("width", totalWidth)
.attr("height", totalHeight);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Drawing this all as one big SVG is antithetical to the responsive design approach that we've tried to take with the rest of the interface. It renders fine given sufficient screen real estate, but on a tablet or phone screen it fails to render properly.


const textAnchor = "end";

mockData.forEach((d, i) => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Rather than appending each boxplot to a single SVG, the code should render an SVG for each boxplot, which should be laid out on the screen using responsive flexbox CSS code. Two columns on full-size devices, one column on smaller screens. Additionally, the size of the boxplots should be scalable to work with smaller screens that don't have 600 horizontal pixels to work with.

If scaling all of the constituent elements of the boxplot would not render appropriately, I'm open to the idea of building the SVG with a fixed width of 600px and displaying a scaled version of the final SVG with instructions for users to click on the image to "zoom in" and view it at full resolution on smaller screens. But I'd prefer to explore a fully responsive approach.

}
});

function drawExampleBoxplot() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

There's a lot of repeated code between this and the code that draws the actual boxplots. Can we create a helper function that just handles the actual drawing and can be fed information to draw either real or example boxplots?

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.

3 participants