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

Conversation

8 participants
@vidartf
Contributor

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

This comment has been minimized.

Show comment
Hide comment
@mpacer

mpacer Jan 18, 2017

Member

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 #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.

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 #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

This comment has been minimized.

Show comment
Hide comment
@mpacer

mpacer Jan 18, 2017

Member

For the purposes of future discussion, some other issues around tags (incl. those already referenced):

CC: @jasongrout and @rgbkrk

From the attic:

Member

mpacer commented Jan 18, 2017

For the purposes of future discussion, some other issues around tags (incl. those already referenced):

CC: @jasongrout and @rgbkrk

From the attic:

@mpacer

This comment has been minimized.

Show comment
Hide comment
@mpacer

mpacer Jan 18, 2017

Member

Possibly related jupyterlab issues:

Member

mpacer commented Jan 18, 2017

Possibly related jupyterlab issues:

@mpacer

This comment has been minimized.

Show comment
Hide comment
@mpacer

mpacer Jan 18, 2017

Member

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).

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

This comment has been minimized.

Show comment
Hide comment
@takluyver

takluyver Jan 19, 2017

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.

Member

takluyver commented Jan 19, 2017

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

This comment has been minimized.

Show comment
Hide comment
@takluyver

takluyver Jan 19, 2017

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.

Member

takluyver commented Jan 19, 2017

@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

This comment has been minimized.

Show comment
Hide comment
@vidartf

vidartf Jan 19, 2017

Contributor

@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.

Contributor

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

This comment has been minimized.

Show comment
Hide comment
@takluyver

takluyver Jan 20, 2017

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.

Member

takluyver commented Jan 20, 2017

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.

vidartf added some commits Jan 20, 2017

Split input on whitespace
Don't allow whitespace in tags. Instead split on whitespace, such that
several tags can be added in one go.
Add dialog for managing tags
This also meant some tags functionality needed to be refactored to avoid
multiple entry points for adding and removing tags.
Add fade-out on overflowing 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

This comment has been minimized.

Show comment
Hide comment
@vidartf

vidartf Jan 20, 2017

Contributor
  • 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.
Contributor

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

This comment has been minimized.

Show comment
Hide comment
@vidartf

vidartf Jan 20, 2017

Contributor

Oh, and:

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

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

This comment has been minimized.

Show comment
Hide comment
@takluyver

takluyver Jan 20, 2017

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.

Member

takluyver commented Jan 20, 2017

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.

@ivanov

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

This comment has been minimized.

Show comment
Hide comment
@takluyver

takluyver Jan 22, 2017

Member
Member

takluyver commented Jan 22, 2017

@takluyver takluyver merged commit 27cfa29 into jupyter:master Jan 30, 2017

4 checks passed

codecov/patch Coverage not affected when comparing 666ecbf...946a4ac
Details
codecov/project 74.92% (+0.00%) compared to 666ecbf
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@vidartf vidartf deleted the vidartf:tags-bar branch Jan 30, 2017

@gnestor gnestor added this to Merged PRs in 5.0 Feb 4, 2017

@ivanov

This comment has been minimized.

Show comment
Hide comment
@ivanov

ivanov Mar 2, 2017

Member

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.

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

This comment has been minimized.

Show comment
Hide comment
@takluyver

takluyver Mar 2, 2017

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.

Member

takluyver 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, 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

This comment has been minimized.

Show comment
Hide comment
@stevengj

stevengj Mar 2, 2017

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

stevengj commented Mar 2, 2017

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

@takluyver

This comment has been minimized.

Show comment
Hide comment
@takluyver

takluyver Mar 2, 2017

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-_./].

Member

takluyver commented Mar 2, 2017

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

This comment has been minimized.

Show comment
Hide comment
@dsblank

dsblank Mar 2, 2017

Member

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?

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

This comment has been minimized.

Show comment
Hide comment
@takluyver

takluyver Mar 2, 2017

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.

Member

takluyver commented Mar 2, 2017

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

This comment has been minimized.

Show comment
Hide comment
@dsblank

dsblank Mar 2, 2017

Member

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

Member

dsblank commented Mar 2, 2017

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

@takluyver

This comment has been minimized.

Show comment
Hide comment
@takluyver

takluyver Mar 2, 2017

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.

Member

takluyver commented Mar 2, 2017

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

This comment has been minimized.

Show comment
Hide comment
@stevengj

stevengj 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.

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

This comment has been minimized.

Show comment
Hide comment
@dsblank

dsblank Mar 2, 2017

Member

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.

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

This comment has been minimized.

Show comment
Hide comment
@takluyver

takluyver Mar 2, 2017

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.
Member

takluyver commented Mar 2, 2017

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

This comment has been minimized.

Show comment
Hide comment
@ivanov

ivanov Mar 2, 2017

Member

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).

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

This comment has been minimized.

Show comment
Hide comment
@minrk

minrk Mar 2, 2017

Member

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

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

This comment has been minimized.

Show comment
Hide comment
@minrk

minrk Mar 2, 2017

Member

@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.

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

This comment has been minimized.

Show comment
Hide comment
@minrk

minrk Mar 3, 2017

Member

Tests in #2249

Member

minrk commented Mar 3, 2017

Tests in #2249

@psychemedia

This comment has been minimized.

Show comment
Hide comment
@psychemedia

psychemedia Apr 5, 2017

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

psychemedia commented Apr 5, 2017

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

@minrk

This comment has been minimized.

Show comment
Hide comment
@minrk

minrk Apr 5, 2017

Member

@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.

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

This comment has been minimized.

Show comment
Hide comment
@psychemedia

psychemedia Apr 5, 2017

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

psychemedia commented Apr 5, 2017

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

@minrk

This comment has been minimized.

Show comment
Hide comment
@minrk

minrk Apr 5, 2017

Member

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

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

This comment has been minimized.

Show comment
Hide comment
@mpacer

mpacer Apr 5, 2017

Member

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.

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

This comment has been minimized.

Show comment
Hide comment
@psychemedia

psychemedia Apr 6, 2017

@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

psychemedia commented Apr 6, 2017

@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 join this conversation on GitHub. Already have an account? Sign in to comment