JovianHQ / jovian-py

Collaboration platform for data science projects & Jupyter notebooks
https://www.jovian.ai
112 stars 31 forks source link

Lab Extension: alertDialog, warnings #166

Closed PrajwalPrashanth closed 4 years ago

PrajwalPrashanth commented 4 years ago
codecov[bot] commented 4 years ago

Codecov Report

Merging #166 into master will not change coverage. The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##            master      #166   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           28        28           
  Lines         1272      1273    +1     
=========================================
+ Hits          1272      1273    +1     
Flag Coverage Δ
#unittests 100.00% <100.00%> (ø)
Impacted Files Coverage Δ
jovian/utils/commit.py 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update eba2ab2...8a7d7f4. Read the comment docs.

PrajwalPrashanth commented 4 years ago

@aakashns Check it out! Apart from the discussion have made few more changes.

@rohitsanj I have made changes to test, can you verify them once.

PrajwalPrashanth commented 4 years ago

@rohitsanj i have added tests to make codecov 100% can you please verify them as well. Let me know if anything can be better there.

PrajwalPrashanth commented 4 years ago

Notebook warning image

Lab Warning image

image

aakashns commented 4 years ago

Thanks for adding screenshots. Some notes on how the UI can be improved:

image

PrajwalPrashanth commented 4 years ago

Thanks for adding screenshots. Some notes on how the UI can be improved:

* The "Warning!" text ideally shouldn't be bigger than the success message itself, to avoid confusion. Please make it smaller (both title & description).

* The exclamation mark in "Warning!" can be removed (causes anxiety). You can replace it with a triangular or circular icon

* Giving it some visual hints will help.  You can use a similar color scheme as we use in the docs.

image

aakashns commented 4 years ago

Lets do the other UI related changes after the refactor of notebook extension ? As design is pending for all dialogs.

Sure, we can merge without the UI changes for now. Let me know here when this is ready for review.

PrajwalPrashanth commented 4 years ago

Its ready. I guess we can have these as a stop gap until we go with designs for extension dialogs. Added apt icons with apt colors.

image image image image

aakashns commented 4 years ago

I've simplified the return statements a bit. Looks like one of the tests need to be updated

aakashns commented 4 years ago

Since the Jupyter extension check is not used for conditional return values, we don't need the Jupyter extension check at all (saving the file is a fairly minor check), so I've removed it and the related tests.

We can add it back later if some use case comes up

PrajwalPrashanth commented 4 years ago

image @aakashns The current master would print out result like this.

Also i think you have updated prettier and the trailing comma and arrow parens got updated.

aakashns commented 4 years ago

Fair point, the URL gets printed twice, once in the log, and then again in the output. However, I think it's OK to return the URL as the output value from jovian.commit. It also gives the end-user a way of checking the result of commit or working with the uploaded notebook programmatically.

Regarding Prettier, we need a add a .prettierconfig file, as we have done in the webapp. I'll add it in a follow up PR