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

Simple implementation of cell tags #2048

Merged
merged 5 commits into from
Jan 30, 2017
Merged

Conversation

vidartf
Copy link
Contributor

@vidartf vidartf commented Jan 18, 2017

This is a basic implementation of a UI for adding cell tags, c.f. #601. The styling, in particular, has a lot of room for improvement (not sure if it hits all the browser targets of the project), but it should at least be functional.

Screenshot:

tags

@mpacer
Copy link
Member

mpacer commented Jan 18, 2017

Great to see movement on this! I was just talking to @jdfreder about resurrecting the cell tagging efforts.

UI features that aren't strictly related to the display that would be helpful:

  • tab completion & suggestion
    • this could probably reuse the UI paradigm that we use to autocomplete with multiple options in other cases
    • these suggestions could be based on a number of things:
      • all tags previously used in the notebook
      • core supported tags (e.g., those like jupyter:hide_cell in nbconvert)
      • tags used by extensions (or at least a list that extensions can write to for getting suggestions)
      • some tags may be more or less appropriate for different kinds of cells (e.g., jupyter:hide_output doesn't mean anything for a markdown cell)
    • for the case of prefixed tags, it may be useful to suggest completions that would apply the prefix if the unprefixed text is present (e.g., hide_celljupyter:hide_cell)
  • "clickable" suggestions (discussed more in A better metadata toolbar #1812)

One downside to this approach to applying tags is that it doesn't have any way to apply to multiple cells at the same time, since multiple cells can be selected, it'd be good to also surface a way to do that. I think that solving that might also suggest a more general UI that would work nicely (especially for applying tags via keyboard shortcut). To be fair, this is a tough problem that none of the other tagging solutions have solved, but it's still worth thinking about.

@mpacer
Copy link
Member

mpacer commented Jan 18, 2017

@mpacer
Copy link
Member

mpacer commented Jan 18, 2017

@mpacer
Copy link
Member

mpacer commented Jan 18, 2017

Does this currently take over the cell toolbar? In that case is this compatible with nbgrader?

One possibility for cells with lots of tags (a problem mentioned in ipython/ipython#6638 (comment)) would be to have a clickable ellipsis that opens a modal editor view (in the vein of the current metadata editor but ideally with a nicer looking UI tailored to this context) that specifically addresses the issue of tagging cells. This type of modal editor view would then allow editing tags on multiple cells at the same time, since the editor's scope is detached from it's physical location for displaying the tags (which are still at the cell level).

@takluyver
Copy link
Member

We know there's a lot more that can be done with the cell tagging UI, but we've been putting off doing it for ages because of making a good UI for it. So Vidar suggested, and I agreed, that he builds a minimal working UI to put into notebook 5.0 (which I hope will be soon) so that we can start playing with tags, and enhance it later.

With that in mind, let's keep this thread focussed on what's the minimum working tag interface that we're happy to land, and leave discussion of how to make a great tag UI for separate issues.

I believe this works as a cell toolbar like any other: you can switch between them using a dropdown. So it shouldn't break nbgrader.

@takluyver
Copy link
Member

@vidartf did you miss pushing a commit? I don't see the tags left-aligned:

screenshot from 2017-01-19 11-41-12

We should make sure that adding/removing a tag sets the dirty flag - at the moment, if I add a tag and then close the browser tab, the notebook is not saved and I don't get a warning.

Unfortunately there's a merge conflict on this now (I guess one of the PRs I just merged touched notebook.js), so it will need rebasing.

@takluyver takluyver added this to the 5.0 milestone Jan 19, 2017
@vidartf
Copy link
Contributor Author

vidartf commented Jan 19, 2017

@takluyver I've resolved the merge conflict. Regarding the styling, I'm not very familiar with all the build steps of this project. I used python setup.py js css to build locally as per the docs. Am I doing it right?

@michaelpacer I definitely agree that some ellipsis/dialog combination is a good solution to the overflow issue. I'll see what I can do. I'm guessing a one-tag-per-line textarea would be a decent minimal implementation for the dialog.

@takluyver
Copy link
Member

Thanks, and I do see the styling correctly now; maybe I had it cached in my browser.

I'd still like it to set the dirty flag before merging, so that the notebook knows it needs to save.

Don't allow whitespace in tags. Instead split on whitespace, such that
several tags can be added in one go.
This also meant some tags functionality needed to be refactored to avoid
multiple entry points for adding and removing tags.
The size of the cell toolbar header is fixed, meaning that overflow
needed to be handled. This also styles a fade out, to help indicate to
the user that the content is overflowing.
@vidartf
Copy link
Contributor Author

vidartf commented Jan 20, 2017

  • Dirty flag is now set whenever any changes are made to metadata.
  • Added a button [...] that can be clicked to show a dialog with a textarea for editing tags.
  • Tags overflowing from the cell toolbar are hidden, with a simple fade-out effect to indicate to the user that content is overflowing.

@vidartf
Copy link
Contributor Author

vidartf commented Jan 20, 2017

Oh, and:

  • Any whitespace (/\s/) in the input will be interpreted as a separator in a sequence of tags to add

@takluyver
Copy link
Member

I've played around with this a bit - it's definitely rough around the edges, but it's up to the level where I'm happy to merge it for 5.0 and let people start playing around with it and improving it. We're keen to do various things with tags, and once we have a basic interface we can start doing that while people polish it.

Copy link
Member

@ivanov ivanov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would like to see some tests of the functionality provided here before merging it. Adding new features without tests is asking for trouble in the long run.

@takluyver
Copy link
Member

takluyver commented Jan 22, 2017 via email

@ivanov
Copy link
Member

ivanov commented Mar 2, 2017

I was not going to respond to this originally, but since it has been referenced elsewhere, I want to weigh in and say that merging features which do not have tests is a mistake.

I was not around when the decision was made to stop adding tests because phantom and Casper have been flaky and irritating, but writing, and more importantly maintaining software which is not tested is even more irritating. If our js testing is flaky, we should put in effort to fix it, or move to something else, but simply giving up on testing altogether is a mistake.

@takluyver
Copy link
Member

I'm well aware that it's a bad idea. But since ~all the developer effort is now focussed on Jupyterlab, nobody has invested the time in reworking the tests in this repo, and I'm not going to ask people to do so to get their pull request merged.

If you have time to spend on it, go for it. I think we were looking into the possibility of using Selenium for tests at one point. @jhamrick do I recall you were using that in nbgrader? @minrk also had some ideas about JS testing.

@stevengj
Copy link

stevengj commented Mar 2, 2017

Has the documentation of the notebook format been updated to explain how the new metadata is stored?

@takluyver
Copy link
Member

Yes, though we may want to make it more precise about which characters are allowed in tags. I'd recommend that we stick to something like [A-Za-z0-9-_./].

@dsblank
Copy link
Member

dsblank commented Mar 2, 2017

Why would you want to limit tags to less than ASCII? Doesn't using Unicode for things like this (especially in 2017) make the most sense?

@takluyver
Copy link
Member

Excluding certain ascii characters (like space, newline and comma) makes it easier to design the tag editing UIs, because you can assume that those characters delimit tags. Opening up unicode brings in a whole range of lookalike characters that might cause confusion, but I'd be OK with adopting certain classes of unicode characters (e.g. the same set that are valid Python 3 identifiers).

I'm inclined to start with a pretty small set of characters, because it's much easier to allow extra characters later than to retrospectively disallow characters people may already have used. All of the use cases I've discussed with people so far can easily be handled with the limited set of ASCII characters I suggested.

@dsblank
Copy link
Member

dsblank commented Mar 2, 2017

Do these usecases involve people using their native language, where their native language is not English?

@takluyver
Copy link
Member

I envisage tag names matching names defined elsewhere, so I don't see it being practical to localise them. This is akin to how we use English words like if and while in programming languages, and in things like pip install notebook - you can't translate install or notebook and expect it to work.

I feel like you're lecturing me on the value of internationalisation, and I resent that - I'm well aware of the importance of that, but this is not the place I think we should be applying it.

@stevengj
Copy link

stevengj commented Mar 2, 2017

I think it would be reasonable to just exclude whitespace and control characters (Unicode categories Zp, Zl, Zs, Cc, Cf), which includes newlines.

@dsblank
Copy link
Member

dsblank commented Mar 2, 2017

I'm not lecturing... it was an honest question. I have no idea what the use cases that you mentioned are. I imagine use cases that are based on user's tagging the cells for their own purposes (like tags in stackoverflow). But perhaps I am mistaken. It sounds like these tags are for more internal uses than what I had imagined.

@takluyver
Copy link
Member

Sorry, my bad. I'm not in a great mood today, and I read your last question in a sarcastic tone.

I envisage there being a range of third party use cases for cell tags - nbval uses tags like nbval-ignore-output, for instance. But I see them primarily as a simple way to use APIs that tools expose, so for the most part I don't see them being localised, any more than you'd localise function names in an API.

Some of the use cases we've envisaged so far include:

  • nbconvert - hide a cell, hide the input leaving the output visible, collapse a cell leaving a way to reveal it
  • nbconvert to latex - markdown cell contains title (or subtitle, abstract...)
  • nbval - check/ignore output from a cell, skip executing a cell, expect a cell to raise an error
  • nbgrader - solution cell, tests cell
  • nbparameterise - cell contains input parameters.

@ivanov
Copy link
Member

ivanov commented Mar 2, 2017

I'm well aware that it's a bad idea. But since ~all the developer effort is now focussed on Jupyterlab

If jupyterlab is really where all of the new effort is, and notebook is getting little attention, why are we adding new features to it, especially untested ones?

nobody has invested the time in reworking the tests in this repo

I recently sped up the js tests, and did not see casperjs failures (though I did see Travis choking). Now that may have been because of travis_retry which I did not realize is being utilized.

If anyone sees JS failures on CI or locally, please document it on Flaky JavaScript tests omnibus ticket (#2243).

@minrk
Copy link
Member

minrk commented Mar 2, 2017

I'd prefer to allow tags to be any unicode with a blacklist of separators, e.g. just comma and space.

@minrk
Copy link
Member

minrk commented Mar 2, 2017

@stevengj thanks for the Unicode character classes, I think that's a good idea.

I'll take a stab at some tests exercising this one.

@minrk
Copy link
Member

minrk commented Mar 3, 2017

Tests in #2249

@psychemedia
Copy link

Apols if this is a naive/inappropriate question - can the cell tags be used to support custom styling of particular cells?

@minrk
Copy link
Member

minrk commented Apr 5, 2017

@psychemedia not at all! In theory, yes, in 5.0, no. We can apply CSS classes to tagged cells (e.g. "cell-tag-" + escaped(tag)), but we don't do it currently.

@psychemedia
Copy link

So I could write a custom css to act on the tags... will explore that if so... :-)

@minrk
Copy link
Member

minrk commented Apr 5, 2017

after we start applying CSS classes based on tags, yes. But we are not yet assigning classes based on the tags (#2371).

@mpacer
Copy link
Member

mpacer commented Apr 5, 2017

Just to point out @psychemedia that there is an example of how to write a template compatible with nbconvert that will watch for tags like this (in order to trigger the toggle-able hiding of different tagged cells) at https://github.com/mpacer/hiding_tags_nbconvert/.

If you want to see the functionality in action go to https://mpacer.github.io/hiding_tags_nbconvert/hide_cells_based_on_tags.html.

@psychemedia
Copy link

@mpacer Thanks...

What I'm aiming for is recreating s/thing like the below (one colour for exercises, one for comments), which used an extension that kludged what element to attach styling to on an old, old notebook variant. From a quick look at the current page structure, I couldn't quite see how to style the CSS, but will take another look.

image

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
5.0
Merged PRs
Development

Successfully merging this pull request may close these issues.

None yet

8 participants