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

Add filtering notebook cells with tags #15157

Open
wants to merge 61 commits into
base: main
Choose a base branch
from

Conversation

HaudinFlorence
Copy link
Contributor

@HaudinFlorence HaudinFlorence commented Sep 26, 2023

This PR aims at adding a way to filter (basically to collapse) cells of a notebook that have specific tags. It provides a dialog listing all the tags of all the cells. The user can choose and check tags in the interface. All the tagged cells will then be collapsed respectively.

filter_cell_collapse_with_tags2

Final styling
Screenshot from 2023-10-29 10-00-05

References

This PR
fixes #12542
fixes #8298

Code changes

In packages/notebook-extensions/src:

  • add a jupyterFrontEndPlugin with a command called filterCells
  • add a new file celltaglist.tsx
  • add button in the common tools associated with filterCells command: when clicking a dialog open displaying the list of available tags for all the cells

User-facing changes

Backwards-incompatible changes

@jupyterlab-probot
Copy link

Thanks for making a pull request to jupyterlab!
To try out this branch on binder, follow this link: Binder

@krassowski
Copy link
Member

Is there an issue discussing design of this feature? I know that tag filtering was previously (3.x) available in Table of Contents. From the UX point of view it appears of high important to indicate that the tag filters that are active. Is the intent to move it back to ToC or keep it on the toolbar?

@krassowski krassowski added this to the 4.1.0 milestone Sep 27, 2023
@HaudinFlorence
Copy link
Contributor Author

HaudinFlorence commented Sep 27, 2023

Is there an issue discussing design of this feature? I know that tag filtering was previously (3.x) available in Table of Contents. From the UX point of view it appears of high important to indicate that the tag filters that are active. Is the intent to move it back to ToC or keep it on the toolbar?

I am not sure there is an issue discussing this design apart from what is said in issue #12542. For the moment the idea is too keep this filtering tool accessible from the toolbar. I would be happy to further discuss UX aspects. Can you please elaborate what you mean by its is of high importance to indicate what tags filters are active. Thanks.

@krassowski
Copy link
Member

In past, when the feature of collapsing cells or collapsing headers was introduced users were very distressed when they triggered it accidentally or triggered it intentionally and forgot that it was active because this lead to impression of:

  • cells being lost from notebook
  • unintentional sharing of a notebook with sensitive information because author thought it is no there

therefore whenever a feature temporarily hiding cells is active it is important to offer an indication of that.

On placement of the feature, we previously had it in the ToC, please see mockups/screenshots from #8566 (comment):

current extension future

and #10346:

image

At the time I suggested that maybe a good placement would be in the cell tools sidebar where the cell tags are added in the first place(#8566 (comment)), but I do not have a strong opinion on it.

@HaudinFlorence
Copy link
Contributor Author

HaudinFlorence commented Sep 27, 2023

@krassowski Thanks for the precision. For the moment the tool is very basic but it is worth to keep this in mind for the future.

@HaudinFlorence HaudinFlorence marked this pull request as ready for review September 29, 2023 16:04
@HaudinFlorence HaudinFlorence force-pushed the add_filtering_notebook_cells_by_tags branch 2 times, most recently from 328ff04 to 0f0f7a4 Compare September 29, 2023 16:06
@krassowski
Copy link
Member

For the moment the tool is very basic but it is worth to keep this in mind for the future.

I think that the UI/UX here should be addressed before merging because:

  • historically there were comments about too many items in the toolbar, so adding a new one by default should have a broad consensus (on the other hand adding the same feature to a sidebar would be a no-brainier)
  • it has a large impact on UX and a minimal solution could be as simple as using the filterDotIcon (if it was to stay in the toolbar)

@HaudinFlorence
Copy link
Contributor Author

HaudinFlorence commented Oct 4, 2023

@krassowski Thanks for the comment. In a future commit, I will either use the filterdot icon option in a first time or move the button to the common tools panel. But first, I am addressing the addition of buttons to validate the tag selection or to clear all filters as it was in the previous filtering tool of jupyterlab 3 version.

tag_cell_filtering_lab4_4

@HaudinFlorence HaudinFlorence force-pushed the add_filtering_notebook_cells_by_tags branch 2 times, most recently from 408bef4 to 823ad8d Compare October 4, 2023 10:08
@HaudinFlorence
Copy link
Contributor Author

HaudinFlorence commented Oct 11, 2023

You can see in the introduction of the PR an updated gif with demonstration of clicking on a button in the common tool, opening a dialog and having the possibility to check some filters (not sure this is the best user experience, maybe having the content of the dialog directly in the common tools would be nicer? but then it is much more complicated to synchronize everything)

For the moment, the case of closing the dialog without having properly clicked on one of the 3 buttons is not addressed (leading to a desynchronisation between the tags checked and the collapsed cells when reopening the dialog).

@HaudinFlorence
Copy link
Contributor Author

The filtering logics has been changed from union to intersection to fit the behavior of the filtering tool that was previously in the table of content of jupyterLab 3.

comparison_filtering_results_jlab3_jlab4

@HaudinFlorence HaudinFlorence force-pushed the add_filtering_notebook_cells_by_tags branch 2 times, most recently from 14a36b1 to fb55843 Compare October 11, 2023 09:51
@HaudinFlorence
Copy link
Contributor Author

HaudinFlorence commented Oct 13, 2023

@krassowski I hope with this last commit we approach something close to what you suggested in terms of UX. Note that when a notebook is opened and when it already has collapsed cells, when opening the filtering dialog, all cells are reset to uncollapsed. I don't know if this is the best behavior to propose.

@HaudinFlorence
Copy link
Contributor Author

bot please update snapshots

@github-actions
Copy link
Contributor

Documentation snapshots updated.

@github-actions
Copy link
Contributor

Galata snapshots updated.

@afshin afshin force-pushed the add_filtering_notebook_cells_by_tags branch from 201186a to 9f6d73b Compare May 17, 2024 10:08
@afshin
Copy link
Member

afshin commented May 17, 2024

bot please update snapshots

Copy link
Contributor

Documentation snapshots updated.

@afshin afshin closed this May 17, 2024
@afshin afshin reopened this May 17, 2024
@krassowski krassowski dismissed their stale review May 23, 2024 09:22

Stale, things changed since

@krassowski
Copy link
Member

It looks like there are conflicts:

image

FYI, force-pushing so many commits makes it really hard to see the most recent discussion due how GitHub works

@HaudinFlorence
Copy link
Contributor Author

It looks like there are conflicts:

image

FYI, force-pushing so many commits makes it really hard to see the most recent discussion due how GitHub works

@krassowski I totally agree that force pushing makes it difficult to see most recent elements of the discussion. If you have any suggestion to make it better and more convenient, please let me know. I will try to change this.

@krassowski
Copy link
Member

Well, I do not know of any mitigation other than using merge commits for resolving conflicts rather than using rebase + force-push (and I completely understand that sometimes force-push is more convenient).

@HaudinFlorence HaudinFlorence force-pushed the add_filtering_notebook_cells_by_tags branch 3 times, most recently from c5cb983 to c85c5c5 Compare May 23, 2024 12:46
@HaudinFlorence
Copy link
Contributor Author

bot please update snapshots

@HaudinFlorence HaudinFlorence force-pushed the add_filtering_notebook_cells_by_tags branch from c85c5c5 to 9e3833f Compare May 23, 2024 12:58
@HaudinFlorence HaudinFlorence force-pushed the add_filtering_notebook_cells_by_tags branch from 9e3833f to 5991580 Compare May 23, 2024 13:00
@HaudinFlorence
Copy link
Contributor Author

HaudinFlorence commented May 23, 2024

@krassowski The PR is now up to date with main. There is only the check_links test failing.

@HaudinFlorence
Copy link
Contributor Author

@ericsnekbytes Thanks for testing the notebook. You comment rises some interesting point. The behavior you reported in your gif is indeed the one expected but it may need some explanations or maybe small changes to make this more intuitive:
when you open the dialog, the form should initially be empty if no "filters" or collapsing actions has been applied previously : the check boxes are not showing the tags of the active cell but the filters that are active on the whole notebook. Maybe we could add in a future PR such a feature, showing the tags of the active cell too.

@ericsnekbytes I am wondering if you had more feedback or thoughts about this point ? Thanks

@ericsnekbytes
Copy link
Contributor

ericsnekbytes commented May 28, 2024

@HaudinFlorence There's definitely a mismatch in expectations for me using this tool.

I was expecting to select tags in the popup dialog, then hit the "collapse cells" button to collapse cells that had that tag applied.

From what I understand now, it looks like the tool intends for you to select tags, and when applied, it will hide cells that do not have that exact combination of tags. I was not expecting the dialog to have any tags already highlighted: I expected them to be blank, then allow me to pick tags for a one-time filtering operation.

Some suggestions:

  • The text "Collapse Cells" on the dialog button maybe add to the confusion about how the tool works. Maybe use "Apply Filter" instead?

  • Both behaviors above are useful (and others too):

    • "Hide cells that have these tags"
    • "Show only cells that have these tags"
    • "Show only cells that have these tags and no others"
    • "Hide cells that have these tags and no others"
    • etc. (More below)
  • So, all of these are different valid behaviors for a cell filtering tool (I roughly lump them into affirmative/negative buckets, with different matching criteria). Some checkboxes or options of some kind to control which type of filtering you want would be great. Maybe "Filter Mode:", with ["Hide Cells", "Show Cells"] options, and "Match Type:", with ["Cell Has Only These Tags", "Cell Has One or More", "Cell Has All or More"] options.

  • An "Expand All" button would work well here, to begin at a "blank unfiltered state" before applying filters.

@HaudinFlorence
Copy link
Contributor Author

HaudinFlorence commented May 29, 2024

@ericsnekbytes Thanks your detailed inputs.

So if I understand correctly, you would have expected to select the tags directly in the dialog and not using the current add tags button in the common tools section of the right panel as it is now? Is is correct ?
About the various suggestions:

  • The text on the button was Filter cells but I recently changed it into Collapse Cells since after some feedback it appears that using filter may be confusing.
  • About the various mode and options, yes maybe it would be worth to have a more complete tool, maybe in a follow up PR
  • This also makes sense to have an expand all buttons.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add filtering notebook cells by tags Add Data Attribute for Cell Tags
8 participants