Taylor-CCB-Group / MDV

GNU General Public License v3.0
9 stars 6 forks source link

Data injection UI for MDV #70

Closed aleql closed 6 months ago

aleql commented 7 months ago

Data injection UI for MDV

netlify[bot] commented 7 months ago

Deploy Preview for mdv-dev ready!

Name Link
Latest commit 771b012247e7860f5b0c88e9b5694f6717c0ac58
Latest deploy log https://app.netlify.com/sites/mdv-dev/deploys/6658dae3f44a040008af55b4
Deploy Preview https://deploy-preview-70--mdv-dev.netlify.app
Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

xinaesthete commented 7 months ago

Thanks @aleql , a good start.

A couple of things I notice. One is that it doesn't adapt to dark mode properly - I have my computer setup to prefer dark mode and it meant that text in the dialog isn't legible.

I've been thinking about starting to use Tailwind with the project - which would involve changes to build configuration etc. Currently there's a bit of a mashup of the original vanilla CSS stuff (some of which is from css files and other bits comes along with uses of createEl() etc), some Material-UI in React (but I've been hesitant to use that heavily and may well remove it entirely), and some little bits of inline CSS in React... I've nothing against using styled-components necessarily - seems reasonably clean, and a reasonable choice here, but it would be could if we could review a bit how we decide what we want to do in terms of these kinds of libraries, I'd welcome your input on that.

xinaesthete commented 7 months ago

Also it should the button should probably be added to the UI only when the project has a certain condition met (like p.setEditable(True) which determines whether the 'save view' button is there - maybe 'editable' also means 'can add data').

aleql commented 7 months ago

@xinaesthete I will investigate the issues with dark mode compatibility and ensure these are addressed in the updated PR. Additionally, I will look into the conditional display of the button as discussed.

Regarding the integration of Tailwind, I am fully on board. Tailwind CSS offers several advantages like a consistent and reusable design system, rapid development and easier maintenance, quick styling without writing custom CSS, and its optimized output results in smaller CSS file sizes and faster page load times. Also, it easily integrates to the already existing JavaScript frameworks used by the project.

The styled components of the FileUploadDialog can be seamlessly refactored into functional components that utilize Tailwind CSS for styling. I am ready to proceed with this as soon as we have Tailwind configured in project infrastructure.

xinaesthete commented 7 months ago

Ok, perhaps I'll expedite adding Tailwind.

xinaesthete commented 7 months ago

I added Tailwind. For now, I'm not including @tailwind base; because that avoids needing to redo a bunch of other style stuff and increases my confidence that there won't be negative side-effects. I haven't tested extensively - only briefly in vite dev.

Another thing about your code - it's possible to separate the class-based MDV glue code from the React code to get fast-reloading happening with the devserver. I'd strongly recommend that. It's a little bit obscure but I've attempted to document somewhat in react.md.

A broader point: it may be that it turns out to be more fiddly adding data to an existing project than it would be to create a new project with data that a user has provided. I think we discussed at some point much earlier the possibility of you working on an interface outside the MDV project view, for viewing and creating projects. The python mdv_desktop class (npm run mdv_desktop or python -m mdvtools.mdv_desktop) currently serves an extremely basic index page with a totally unstyled list of projects (from the /projects/ route), along with routes for /project/project_name. The projects themselves can then be used with the devserver http://localhost:5170/?dir=/project/project_name which is how I do most of my development.

@jayeshhireox is working on something that will manage a set of projects in a database, which should have a similar route for /projects/ listing the projects accessible to the current user etc. I think that it might be easier and more productive for you to work on an interface for that which doesn't necessarily need to integrate with the main MDV project front-end code. We'll need to tweak vite config a bit to get that going, but it shouldn't be difficult.

xinaesthete commented 7 months ago

I didn't manage to make a very clean job of it with git but I've merged changes and made a few more alterations to allow HMR to work & start implementing Tailwind. Hope to have a sensible API route for this soon.

aleql commented 7 months ago

Hi Pete,

Thanks for adding Tailwind to the project. I've updated the FileUploadComponent on my branch with the latest changes as of April 16, 2024, switching out the styled components for Tailwind ones.

Regarding the dark theme, while Tailwind does support dark mode natively when you configure darkMode: 'selector' in the Tailwind config, there was an issue with theme recognition. This stemmed from the class names "Light" and "Dark" being capitalized. To align with Tailwind's requirements and standard practices, which call for lowercase ,I've adjusted the theme settings to use lowercase class names in these updates.

xinaesthete commented 7 months ago

Thanks @aleql that all looks fine - only minor thing I notice package-lock.json ended up with duplicate entries for 'react-dropzone' & 'styled-components' after your merge (or maybe mine). Suggest probably just manually deleting those lines (or I will when I merge). Once I've cleaned up/committed my local state I'll merge your changes and leave this PR open.

At some point soon I should really push to main...

xinaesthete commented 7 months ago

Also I think some of the weirdness in changes I made to how things are imported / exported, and that whole BaseDialog.experiment thing is probably unnecessary, we might be able to tidy that up soon. I'm working on other dialogs and should be a bit clearer on that soon.

aleql commented 7 months ago

Hi Pete,

I've just added the functionality for adding a new data source to the FileUploadComponent. Right now, it sends a POST request via axios , which allows for data upload progress tracking, to flaskURL/projectName/add_datasource with the necessary details specified in Jayesh's document. The backend is expected to respond with validation data including rows, columns, and size.

While we're defining this endpoint, I've also set up a temporary confirmation request to flaskURL/projectName/confirm_datasource. This request sends a body with a confirmed field to handle user confirmations.

To manage these API requests, the component needs access to the Flask URL and the current project name. To address this, I've implemented a ProjectProvider. This provider is especially crucial for components like the FileUploadComponent, which is instantiated from ChartManager.js and doesn't get its parameters directly from static_index.ts.

As the project is moving towards a single React root, integrating ProjectProvider might prove useful across all project components in the future, providing necessary variables universally by wrapping the root. Please let me know what you think of this approach.

Lastly, I've added a small check to ensure that only CSV files can be selected for upload for now.

Looking forward to your thoughts

xinaesthete commented 6 months ago

Hi, it's getting there, will need a few changes.

Where we have 'boolean'-ish flags in the form data, the way that the logic in the server works is actually that any value (even empty string) for the given field is treated as true and otherwise it's false.

i.e.

replace = True if "replace" in request.form else False

This is somewhat similar (at least in my mind) to having a URL search parameter ?replace as opposed to ?replace=TRUE, or an attribute on an HTML element that is just a flag with no associated value. I'm not sure whether this is a good or bad practice etc. Could easily be changed.

I had to change it so that backend flag wasn't set in order to avoid invoking @jayeshhireox's code which isn't yet running out-of-the-box.

Another thing I didn't pick up on when I read the endpoint documentation is that view is not boolean but an (optional) string, indicating the name of a view to which the new datasource should be added... so right now, it makes a "view" (MDV jargon for the things that appear in the top-left dropdown) called 'TRUE' as that's the value in the form data.

view = request.form["view"] if "view" in request.form else None

n.b. if there was no form value for this, the current logic is that it will explicitly use None, meaning that it is not automatically added to any view, either new or existing, but it can still be added to new views added by the user with the "+" "create new view" button to the left of the upload button. (This is different to the python MDVProject.add_datasource() method which uses 'default' as a default value, but can have None passed explicitly - again, this is not necessarily the best/most intuitive behaviour).

When it uploads successfully, pressing the 'confirm' button currently tries to call the non-existent /confirm_datasource route, so the user gets an axios 404 error in the dialog, but refreshing the page manually does result in the project having new data added to it.

I'm noticing a couple of CSS oddities when I run through the mdv_dekstop server that seem like they might be a me problem I didn't notice earlier... also unfortunately the useProject() isn't quite managing to get the right route when running through the dev server - e.g. it tried to call http://localhost:5170/pbmc3k_bak/add_datasource, missing project/. I apologise again for some of the flakiness around those routes.

Hopefully most of this is reasonably straightforward.

Cheers, Peter

aleql commented 6 months ago

Hi Pete, I've pushed the latest changes to Git regarding file validation on the front end. This validation is handled through a web worker that streams the file, allowing us to show the user the file name, size, number of rows, and number of columns. Additionally, itnow displays a scrollable interface with each column's name, inferred type, and a value from the second row as an example. This helps users validate the file before uploading.

To accommodate the new scrollable interface, I implemented the onResize method in Dialog.js, which allows the dialog to be resized in this specific circumstance.

xinaesthete commented 6 months ago

Thanks @aleql , sounds promising. I notice the failed netlify workflow because of a minor syntax error in ChartManager.js; there was an unbalanced set of braces around where the upload button is added (nb, I think that button should only be visible if the project is editable, i.e. with p.setEditable(True) in Python, which sets 'permission': 'edit' in state.json).

To do an equivalent check to that workflow (tsc & vite build) locally, you can npm run vite-build (at some point I'll probably replace build with that and get rid of webpack).