fohrloop / dash-uploader

The alternative upload component for python Dash applications.
MIT License
144 stars 30 forks source link

Fix className logic #61

Closed jayeclark closed 2 years ago

jayeclark commented 2 years ago

Signed-off-by: Jay Clark jay@jayeclark.dev

Hi there! I'm working to fix #60 - well, I've fixed the logic already but am wondering if you have any suggestions on the best way to test/validate the fix? I've been having some issues following the Contributing guidelines (getting hung up on the python -m pip install -e step.)

I noticed there weren't any tests for that file and wondered if you'd like me to add one using React's built-in testing library?

fohrloop commented 2 years ago

Hey! This is awesome!

I'm answering from mobile so therefore just a quick answer, but:

1) yes, it would be nice to have this fixed in the latest branch (flow-dev). This could be merged to dev and I could then merge it forward to flow-dev (which has still many broken parts)

2) it would be absolutely really awesome to have some React tests! I was thinking this today but as I have so little front-end experience I decided not to start writing tests for frontend yet myself. It would probably make testing the component a lot faster. Currently there are only "full stack" tests done with pytest, which utilize the built js files.

I am sorry to hear about problems with the contributing instructions! Let me see the output and I can help you further! :)

fohrloop commented 2 years ago

For the testing part, the component can only be paused, uploading or completed or default (if I remember right). And on top of that the state could be "hovered" or "disabled". At least it would be beneficial to test combos with "hovered" and "disabled". Not sure how it would be possible to simulate mouse hover with React tests, or even if it is possible at all. If not, then this could be added to the pytest tests.

fohrloop commented 2 years ago

The front-end tests could alter the state of the component directly (without any real uploads) and just check if the class changes.

jayeclark commented 2 years ago

Sounds great - I’ve signed off for the day here but can add that test file tomorrow morning.

As for the install issues, I’m still troubleshooting on my end, almost certainly it’s user error on my part. I’ve been getting the same ‘path not found’ error on the pip install step no matter what I try (even without the [dev] flag), and I probably need to read up on the documentation a bit to figure out where I’m going wrong (because I have tried just about every reasonable path I can think of, and haven’t had this issue otherwise in terminal!)

fohrloop commented 2 years ago

What do you have in the pip install after the -e flag? It should be relative or absolute path pointing to folder with the setup.py. Let me hear if I can help you in any way.

jayeclark commented 2 years ago

Well, I think I got past the original issue I was encountering, but now I'm stuck on this:

Screen Shot 2022-01-02 at 4 38 27 PM
fohrloop commented 2 years ago

You probably meant to use <folder>[dev] (note: no space, no quotes and added brackets) instead of <folder> "dev" to install the development dependencies. Now the "dev" was interpreted to be the package version.

jayeclark commented 2 years ago

Hmm. That's what I had been trying originally... in various formats in case I misinterpreted the instruction.

Screen Shot 2022-01-02 at 4 29 33 PM

.

fohrloop commented 2 years ago

Oh, now I have a guess! See: https://stackoverflow.com/questions/30539798/zsh-no-matches-found-requestssecurity

jayeclark commented 2 years ago

I did finally get the pip install to work -- but I was able to verify that the logic works using the testing library as well. I may add additional tests in a separate PR later in the week, but for now I wanted to keep it simple and just focus on tests related to the original request (fixing the logic of getClass()).

fohrloop commented 2 years ago

I cherry picked these in 11c2022149388acc22cd3a9de2b40eb62c543a41 .. 65d7e2a71bf2196f45eeb4b0bb2f7f4aae2b76bf to be part of the flow-dev branch, too.