cidgoh / DataHarmonizer

A standardized browser-based spreadsheet editor and validator that can be run offline and locally, and which includes templates for SARS-CoV-2 and Monkeypox sampling data. This project, created by the Centre for Infectious Disease Genomics and One Health (CIDGOH), at Simon Fraser University, is now an open-source collaboration with contributions from the National Microbiome Data Collaborative (NMDC), the LinkML development team, and others.
MIT License
90 stars 23 forks source link

Refactor for library / interface split #329

Closed pkalita-lbl closed 1 year ago

pkalita-lbl commented 2 years ago

There are a lot of changes here so I'd be glad to walk anyone through them in more detail. Just reach out to me if that would be helpful for you.

The changes fall into these categories:

The development workflow with this branch is:

A few points that may warrant discussion:

  1. The fact that the images used in the Getting Started carousel need to be bundled into the core library is a bit awkward and could inflate downstream projects. It is tree-shakable, so if a client isn't using the Toolbar class, they won't incur the extra bundle size. Regardless, even for ease of updating I wonder if that carousel could be replaced by a link to documentation on the GitHub Wiki.
  2. You'll notice that web/templates/menu.js now includes information about the schema and export formats for each folder. This replaces the SCHEMA and EXPORT_FORMATS global variables that were previously used to manage that information. Instead of having the Toolbar class have knowledge about a specific file system structure, I added these two fields to the objects in menu.js that can either be inlined values (an object for schema or an array for exportFormats) or functions that provide such values. The dynamic imports in menu.js are a little awkward looking, but at least it keeps Toolbar's interface quite generic. Curious if anyone has alternate ideas.
  3. I have not updated scripts/linkml.py. A schema JSON file can be generated from the LinkML YAML on the command line with: gen-linkml --format json --mergeimports schema.yaml > schema.json.
  4. I attempted to leave the public interface of the DataHarmonizer class the same as it was before as much as possible. However I did attempt to remove any references to DOM elements in DataHarmonizer that it didn't create itself. That required replacing changeColVisibility with 4 more specific methods (showAllColumns, showRequiredColumns, showRecommendedColumns, showColumnsBySectionTitle) Personally I prefer this type of more explicit interface, but that's open to discussion. (It may also be worth in the future looking at what methods and properties on DataHarmonizer should be public and make the rest private.)

Fixes #312

ddooley commented 2 years ago

I'll try to have a peek/runthrough tomorrow, and then follow up with questions.

ddooley commented 2 years ago

I checked it out and also consulted with the team and happy to see its 98% there! It seems like on my local test of web/dist/index.html, as well as the localhost http://localhost:8080/ yarn version:

I could find time this week to try to resolve those things. No other dev. planned for this week.

pkalita-lbl commented 2 years ago

Thanks for taking a look and testing! I'm happy to look into the issues you found but I'm not fully understanding the steps to reproduce the problems you saw. Here's what happens when I use the Next Error button: https://user-images.githubusercontent.com/102547565/178542044-464e91c4-b834-4acf-9bf9-538dc5a89306.mov

And vertical scrolling: https://user-images.githubusercontent.com/102547565/178542754-9c67e6c0-2bd7-4693-b980-633a872aadcd.mov

ddooley commented 2 years ago

Well that's working perfectly at your end! Are you on a Mac btw, and using Chrome? And was that via http://localhost:8080/ or via file explorer? e.g. file:///Users/damion/Documents/GitHub/DataHarmonizer/web/dist/index.html after the yarn web.build is done?

pkalita-lbl commented 2 years ago

Yes I am on a Mac and using Chrome. I recorded those while using the webpack-dev-server (localhost:8080) but I just tried the bundled version (opening web/dist/index.html) and saw the same behavior.

Maybe we should just do a quick call some time to see what's going on? It's entirely possible I didn't properly document some part of the workflow.

ddooley commented 1 year ago

Hi Patrick, I see you've made this all up to date with DH v1.3.4 . I'm merging with master now!

ddooley commented 1 year ago

Should I just drop yarn.lock or try repairs in dependabot list: https://github.com/cidgoh/DataHarmonizer/security/dependabot

pkalita-lbl commented 1 year ago

Ah sorry I don't have permission to view this repo's dependabot alerts. Don't remove yarn.lock that's important to keep checked in. If the issues have a fix dependabot will generally open a PR automatically, and I generally merge those without much scrutiny. Sometimes dependabot flags issues that don't have a fix published yet, and in that case I'm not sure if there's much else to do other than read what the issue is and see if this code could be affected by it.

ddooley commented 1 year ago

Maybe you have permission now to view dependabots? I added you as a write contributor.

d.