Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Minor fixes and improvements on frontend extension #1178

Merged
merged 15 commits into from
Dec 2, 2023

Conversation

mahendrapaipuri
Copy link
Contributor

@mahendrapaipuri mahendrapaipuri commented Nov 28, 2023

This is follow up of #1163 that mainly attempts to fix remarks made here.

@parmentelat @mwouts Do you think settings page has better readability now?

The thing is Settings use rjsf behind the scenes and we cannot use text formatting like bold, italic, etc. Even if we use HTML tags, they will be escaped and rendered literally. So, I used a null JSON type to show message about browser refresh at the end in a separate line.

@mwouts Before we merge this, we need to update the documentation unless you are working on it. I can update the screenshots and basic documentation of extension and maybe later you can expand on it. What do you think? But before its better we agree on settings UI.

Copy link

codecov bot commented Nov 28, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (2bf03e6) 96.74% compared to head (60ab3ee) 97.73%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1178      +/-   ##
==========================================
+ Coverage   96.74%   97.73%   +0.98%     
==========================================
  Files          29       29              
  Lines        4456     4456              
==========================================
+ Hits         4311     4355      +44     
+ Misses        145      101      -44     
Flag Coverage Δ
external 75.05% <ø> (ø)
functional 88.56% <ø> (ø)
integration 77.31% <ø> (ø)
unit 66.56% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@mwouts
Copy link
Owner

mwouts commented Nov 28, 2023

Hi @mahendrapaipuri , thank you for being back to this! Well, if I can request a few more changes, this is what I think we could do:

  1. Move the New Text Notebook sub-menu under File/New, create a Jupytext sub-menus with the rest of the current Jupytext Menu, as in the previous Jupytext Menu (well, if you agree that these operations are well fitted in the File menu?)
  2. I like the configuration as it is on main. I think the introductory paragraph is clear enough. If I were to change anything, I would replace kernel variants by kernel languages (since we iterate over languages rather than kernels right?). And I do not think that we need to be able to configure the name for the new launcher entry (the default, 'Jupytext', seems perfect to me!)
  3. After that yes indeed we will have to update the screenshots in the documentation. I was wondering if it would make sense to take the screenshots from the automated tests?
  4. Yes we could remove the New undefined command (but I guess that is already in the current PR?)

BTW I have released a RC as of yesterday: https://pypi.org/project/jupytext/1.16.0rc0/. So far everything works great! I plan to do the actual release over the next week-end if that is fine with you.

@mahendrapaipuri
Copy link
Contributor Author

Move the New Text Notebook sub-menu under File/New, create a Jupytext sub-menus with the rest of the current Jupytext Menu, as in the previous Jupytext Menu (well, if you agree that these operations are well fitted in the File menu?)

I see that you would like to have the legacy UI. I thought having a dedicated Jupytext menu can improve the visibility accessibility. I agree moving New Text Notebook sub-menu under File>New is logical but I feel that rest of Jupytext sub-menus like pairing, metadata, FAQ, etc are not well suited for File menu. Anyways you know the user base more than me and if you think users will find it more easy having the Jupytext menu under File, we can move them.

I like the configuration as it is on main. I think the introductory paragraph is clear enough. If I were to change anything, I would replace kernel variants by kernel languages (since we iterate over languages rather than kernels right?). And I do not think that we need to be able to configure the name for the new launcher entry (the default, 'Jupytext', seems perfect to me!)

I agree that introductory paragraph has clear information. But I tend to agree with @parmentelat as well as impatient users do not tend to read dense text (that includes me. hahah!). Unfortunately, we cannot really format the text to split in multiple lines, so, I tried to modify it to make it an easy read. What do you think?

About the kernel variants, we do iterate over kernels and and we only use languages to get necessary metadata of kernel. So, if a user installs two variants of bash kernel, Jupytext menu will add both variants although both use shell language. Does it make sense? But I would trust you more on wording than myself (living in France does not help my English. Hahah!).

After that yes indeed we will have to update the screenshots in the documentation. I was wondering if it would make sense to take the screenshots from the automated tests?

This is a good idea. We just need to add link checker on md files in CI to ensure we do not have any broken links when we rename UI test files.

Yes we could remove the New undefined command (but I guess that is already in the current PR?)

Yes, this is taken care of.

BTW I have released a RC as of yesterday: https://pypi.org/project/jupytext/1.16.0rc0/. So far everything works great! I plan to do the actual release over the next week-end if that is fine with you.

That's great news. Congrats!! Once we agree, the changes the changes we are talking about should not take too much time. I can quickly update the PR for you to review.

@parmentelat
Copy link
Contributor

I gave a quick try with commit 1634da2

I agree that having the 'New Text Notebook' submenu in the File submenu right under New would be a better fit, as this is the place where people will look first; the rest of the Jupytext submenu is fine like it is now; this is in an ideal world, if it's too complex to implement the current setup is fine too :)

I also agree with @mwouts that the ability to configure the 'Category' in settings brings no added value and we should get rid of it (or else, at least mention this is about the launcher entry)

apart from that the settings pane is fine now, shorter to read, and I like that the warning on reloading comes last

thanks a lot again @mahendrapaipuri for your patient work :)

@mahendrapaipuri
Copy link
Contributor Author

Alrighty! Cheers @parmentelat for testing the patch.

Here are the latest changes

  • Moved New Text Notebook under File and left rest in Jupytext menu
  • Removed Category Settings config
  • Using screenshots from tests in docs

@mwouts @parmentelat Let me know what do you think!!

@parmentelat
Copy link
Contributor

yes !! that looks just perfect to me :)

all that remains now - food for another time I mean ;-) would be to have the 'right' kernel selected by default
I was doing my test at 100 mph and I clicked enter and tried to write some javascript in a .js, but the kernel was python...
really no big deal of course :)

as far as I am concerned we are a go 👍

@mwouts
Copy link
Owner

mwouts commented Nov 29, 2023

Awesome! Thank you both again. I might not have much time for the rest of this week but this is really promising.

About the kernel variants, we do iterate over kernels and and we only use languages to get necessary metadata of kernel. So, if a user installs two variants of bash kernel, Jupytext menu will add both variants although both use shell language. Does it make sense? But I would trust you more on wording than myself (living in France does not help my English. Hahah!).

Oh that's an interesting point! In the menu we just use the language and extension, is this correct? Imagine that I have 12 Python kernels, I'd prefer not to get 12 entries for Python percent notebooks? (I think I saw just one when I tested).

"All the available kernel variants will be added to launcher and jupytext menu"

Provided that my understanding above is correct, I would say that we do not even need to mention that. Most users will only have Python installed. And the other ones will have noticed that their favourite language is already available in the New Text Notebook menu.

Thanks for updating the docs too, I like the automated screenshots!!

@mahendrapaipuri
Copy link
Contributor Author

Oh that's an interesting point! In the menu we just use the language and extension, is this correct? Imagine that I have 12 Python kernels, I'd prefer not to get 12 entries for Python percent notebooks? (I think I saw just one when I tested).

Oh, yes! You are right!! Adding all available kernel variants was my first implementation and then after our discussions we have changed it to pick up kernel languages. So, I was wrong in my previous comment. If there are multiple kernels with same language, only the last one that appears in kernelspecs will be picked. I have also made a small modification to use generic language name instead of kernelspec name in launcher to make Jupytext entries kernel agnostic.

Provided that my understanding above is correct, I would say that we do not even need to mention that. Most users will only have Python installed. And the other ones will have noticed that their favourite language is already available in the New Text Notebook menu.

Implemented this change in the latest commit.

New additions:

  • I have added a new optional dependency for UI tests and installing Calysto bash kernel to ensure that extension works with non Python kernels
  • Added a new UI test that creates bash text notebook and updated snapshots

@mwouts
Copy link
Owner

mwouts commented Dec 1, 2023

Thank you again @mahendrapaipuri ! Yep the settings page will be even lighter this way, and still as useful.

I have been thinking more about the location of the Jupytext Menu, and yes I am afraid I do miss the previous location (everything under File). My arguments for asking that are the following:

  1. Pairing a notebook to another format is a File operation, since it decides to what set of files the notebook will be written.
  2. I have a preference for Jupytext being discrete and impacting the users as little as possible. In some cases Jupytext is installed in an environment used by multiple users, and it should not distract the users that have not made an active choice in getting it.
  3. 'Jupytext' is not a standard set of operations that I would expect in an application menu.

Happy to chat more about that! And looking forward to integrating this final touch!!

mahendrapaipuri added a commit to mahendrapaipuri/jupytext that referenced this pull request Dec 1, 2023
* See [discussion](mwouts#1178 (comment))

Signed-off-by: mahendrapaipuri <[email protected]>
@mahendrapaipuri
Copy link
Contributor Author

Pairing a notebook to another format is a File operation, since it decides to what set of files the notebook will be written.

Agree, this is a valid argument.

I have a preference for Jupytext being discrete and impacting the users as little as possible. In some cases Jupytext is installed in an environment used by multiple users, and it should not distract the users that have not made an active choice in getting it.

Alright! I see what you mean. I tried to redo the menu items like in the legacy notebook extension.

Let me know what do you think. You can check snapshots for new UI of the menu.

mwouts pushed a commit to mahendrapaipuri/jupytext that referenced this pull request Dec 2, 2023
* See [discussion](mwouts#1178 (comment))

Signed-off-by: mahendrapaipuri <[email protected]>
@mwouts mwouts force-pushed the imrpovs_extension branch from 7a5d369 to bf3784e Compare December 2, 2023 11:43
mwouts pushed a commit to mahendrapaipuri/jupytext that referenced this pull request Dec 2, 2023
* See [discussion](mwouts#1178 (comment))

Signed-off-by: mahendrapaipuri <[email protected]>
@mwouts mwouts force-pushed the imrpovs_extension branch from bf3784e to f3b42b7 Compare December 2, 2023 12:04
mwouts pushed a commit to mahendrapaipuri/jupytext that referenced this pull request Dec 2, 2023
* See [discussion](mwouts#1178 (comment))

Signed-off-by: mahendrapaipuri <[email protected]>
@mwouts mwouts force-pushed the imrpovs_extension branch from f3b42b7 to bf9eb71 Compare December 2, 2023 12:06
Copy link

github-actions bot commented Dec 2, 2023

Thank you for making this pull request.

Did you know? You can try it on Binder: Binder:lab or Binder:notebook.

Also, the version of Jupytext developed in this PR can be installed with pip:

pip install git+https://github.com/mahendrapaipuri/jupytext.git@imrpovs_extension

(this requires nodejs, see more at Developing Jupytext)

@mwouts
Copy link
Owner

mwouts commented Dec 2, 2023

Hey @mahendrapaipuri , sorry for the repeated updates, I am trying to get the comment PR action to work on forks, and it's not obvious 😄 I've had to use pull_request_target and now I see that the repository is not what I think it would be...

(I have opened thollander/actions-comment-pull-request#324)

@mahendrapaipuri
Copy link
Contributor Author

@mwouts You can use ${{ github.event.pull_request.head.repo.full_name }} for the origin repo name of PR and ${{ github.event.pull_request.head.ref }} for the branch of origin repo.

Check jupyterlab-gitlab repo on how it is being done. Here it is in action

@mwouts
Copy link
Owner

mwouts commented Dec 2, 2023

That's really impressive @mahendrapaipuri , you have the answer to everything!! Thanks 🙏

mwouts pushed a commit to mahendrapaipuri/jupytext that referenced this pull request Dec 2, 2023
* See [discussion](mwouts#1178 (comment))

Signed-off-by: mahendrapaipuri <[email protected]>
@mwouts mwouts force-pushed the imrpovs_extension branch from bf9eb71 to 5fcfba3 Compare December 2, 2023 17:45
Signed-off-by: mahendrapaipuri <[email protected]>
* Remove Category settings option

Signed-off-by: mahendrapaipuri <[email protected]>
* Save full page screenshot so that we can use them in docs

Signed-off-by: mahendrapaipuri <[email protected]>
* Titles are already self explanatory

Signed-off-by: mahendrapaipuri <[email protected]>
* Install Calysto bash kernel for UI tests

* To ensure non standard kernels are working as well

Signed-off-by: mahendrapaipuri <[email protected]>
* Avoid using kernelspec display names

Signed-off-by: mahendrapaipuri <[email protected]>
Signed-off-by: mahendrapaipuri <[email protected]>
Signed-off-by: mahendrapaipuri <[email protected]>
* See [discussion](mwouts#1178 (comment))

Signed-off-by: mahendrapaipuri <[email protected]>
@mwouts mwouts force-pushed the imrpovs_extension branch from 5fcfba3 to 60ab3ee Compare December 2, 2023 17:55
@mwouts
Copy link
Owner

mwouts commented Dec 2, 2023

Great work again! Thank you @mahendrapaipuri .

@mwouts mwouts merged commit 7223622 into mwouts:main Dec 2, 2023
31 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants