Paneltours - Githubissues

microbiome / iSEEtree

extension of iSEE for TreeSE

https://microbiome.github.io/iSEEtree/

3 stars 2 forks source link

Paneltours #26

Closed ElySeraidarian closed 5 months ago

ElySeraidarian commented 5 months ago

This PR is related to #24, this not all operationnal yet but it will be easier to discuss about it and make the fixes.

I have been able to add most of the panels asked HOWEVER :

Some panels are not displayed at the right place for some reason ( middle of the screen) -> this does not concerns selectInput items. (This issue is also present in plotRowTree)
- Feels like every single panel of plotRowTree is mapped (when launching the app every question mark has a panel) so I was wondering which one should I be adding
- I have no idea where is the RDA plot so this is pretty hard to know if this is working & having accurate description

ElySeraidarian commented 5 months ago

By the way, macOS build was successul (ubuntu still not working but my side issue I will try working on different branch next time)

RiboRings commented 5 months ago

Hi, thanks for the PR!

Great catch! It turns out .selectInput is a little bit special. If you check the source of for example reducedDimPlot panel, you will see that other tours don't need the "selectize" string in the title. I added a working example in the checkbox observer of the AbundancPlot panel, you can do for the rest.
You are right! Would you like to try to make a tour sequence? For example, if you click the question mark on the upper right corner of reducedDimPlot, it will guide you through all the parameters sequentially. I haven't figured it out myself yet, but it would be cool to have (no need to do if you don't find it interesting).
You can see the example in the RDAPlot vignettes to make it appear.

Extra: could you keep lines less than 50 characters (vertical line in RStudio) and use indentation with multiples of 4 spaces? These are Bioconductor standards, you can run BiocCheck::BiocCheck(".") to see more tips/good practices.

ElySeraidarian commented 5 months ago

First of all thanks for this useful return !

I have made adjustments to make sure BiocCheck runs with the lowest amount of warnings (line lengths & indentation).

Thanks to your example I have been able to place panels at the right place for almost every plot however I still have one issue : it does not work for some parameters of RDAPlot. I believe this is because we are using fields like "add.ellipse" and not something like "add_ellipse" because it's the same code but works for "colour_by" and not "add.ellipse" so I believe this would be the only possibility.

Moreover, as said previously I also see a tour sequence for RowTree Plot as in reducedDimPlot panel so maybe I'm really missing something... Sorry for that.

RiboRings commented 5 months ago

Great job!

The panel tours for RDAPlot are correctly placed now. Using \ to escape the dot fixed the issue.

I think you are right that all panel tour sequences are already there.

You can do a final check and label the review points as solved. When you are satisfied with everything, feel free to merge to devel.