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 search and tag to metadata #985

Merged
merged 37 commits into from
Nov 9, 2020
Merged

Conversation

marthacryan
Copy link
Member

@marthacryan marthacryan commented Oct 20, 2020

Using the code from https://github.com/jupytercalpoly/jupyterlab-code-snippets as a basis to add ability to search / filter by tags in metadata explorer.

  • Add search bar and tag viewer
  • Add ability to search by name of metadata
  • Add ability to add a tag in the metadata editor
  • Add ability to filter by tags

Fixes #277

This is the new feature:
search

Developer's Certificate of Origin 1.1

   By making a contribution to this project, I certify that:

   (a) The contribution was created in whole or in part by me and I
       have the right to submit it under the Apache License 2.0; or

   (b) The contribution is based upon previous work that, to the best
       of my knowledge, is covered under an appropriate open source
       license and I have the right under that license to submit that
       work with modifications, whether created in whole or in part
       by me, under the same open source license (unless I am
       permitted to submit under a different license), as indicated
       in the file; or

   (c) The contribution was provided directly to me by some other
       person who certified (a), (b) or (c) and I have not modified
       it.

   (d) I understand and agree that this project and the contribution
       are public and that a record of the contribution (including all
       personal information I submit with it, including my sign-off) is
       maintained indefinitely and may be redistributed consistent with
       this project or the open source license(s) involved.
Siteproxy
Martha Cryan added 3 commits October 20, 2020 13:42

Unverified

This commit is not signed, but one or more authors requires that any commit attributed to them is signed.
@elyra-bot
Copy link

elyra-bot bot commented Oct 20, 2020

Thanks for making a pull request to Elyra!

To try out this branch on binder, follow this link: Binder

@marthacryan marthacryan marked this pull request as draft October 20, 2020 21:08
Martha Cryan added 5 commits October 22, 2020 14:11

Unverified

This commit is not signed, but one or more authors requires that any commit attributed to them is signed.

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
@marthacryan marthacryan added the component:code-snippets Code Snippet Extension label Oct 22, 2020
@marthacryan marthacryan marked this pull request as ready for review October 22, 2020 20:32
@marthacryan marthacryan changed the title [WIP] Add search and tag to code snippets Add search and tag to code snippets Oct 22, 2020
@marthacryan
Copy link
Member Author

marthacryan commented Oct 22, 2020

[EDIT: bug is fixed now]
Found a bug:
When searching, if a snippet is hidden because it doesn't match the search, when it reappears after erasing the search, the code area is blank. So for example, this is the code snippets before search "is":
image

After removing search "is":
image

@marthacryan marthacryan requested a review from ptitzler October 22, 2020 20:51
@marthacryan
Copy link
Member Author

Fixed bug mentioned above - should be fully ready to play around with if anyone has time.

@ptitzler
Copy link
Member

Will test today

@ptitzler
Copy link
Member

ptitzler commented Oct 23, 2020

  • Is cell tags a good label? Not really sure what it means in this context

image

  • Perhaps we should auto-create tags for the snippet's language, since that is likely a commonly used filter criterion? (the current free text search doesn't cover the snippet language)

Edit 1: If language tags are implicitly defined (e.g. are automatically derived from the language property) they probably don't need to be displayed in the editor (like shown below), just in the selection dialog in the snippet management widget.

image

@ptitzler
Copy link
Member

I'm running into an issue where a newly created code snippet isn't displayed in the list on the left hand side unless the screen is manually refreshed.

@ptitzler
Copy link
Member

ptitzler commented Oct 23, 2020

What happened to drag-and-drop?

Edit 1: If this is delivered in another PR (aka out of scope for this one) please ignore.

@ptitzler
Copy link
Member

Would be great if the list could be sorted alphabetically

image

@ptitzler
Copy link
Member

image

  • Unlike other pop-up messages, this one looks like a default alert window.
  • The duplicate tag error message text should probably be a bit more formal, such as A tag with this label already exists.
  • Perhaps we can just ignore the tag silently in this case, since it's already included in the set?

@ptitzler
Copy link
Member

Not sure tags should be case sensitive from a conceptual point of view. For example, what's the difference between "MARKDOWN" and "markdown" as far as the user is concerned?

image

@ptitzler
Copy link
Member

ptitzler commented Oct 23, 2020

Based on set of randomly created snippets, I'm not sure I understand the tag filter principle

snippets.tar.gz

  • Initially all (?) tags are displayed
    image

  • Choose a tag - in this case only one snippet is matched (note that the python tag is no longer displayed)
    image

  • Choose a second tag from the list; now more snippets are shown and more tags are displayed
    image

My suggestion would be to make the behavior more predictable:

  • display all tags by default
  • option A: if a tag is selected only snippets referencing the tag are displayed; the list of tags is updated to only include the ones that are referenced in the snippets that meet the currently selected tag(s) => tags are ANDed: tag_a AND tag_b ...
  • option B: if a tag is selected only snippets referencing the tag are displayed; the list of tags is not updated => tags are ORed: tag_a OR tag_b

Copy link
Member

@ptitzler ptitzler left a comment

Choose a reason for hiding this comment

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

See earlier review comments.

Copy link
Member

@ajbozarth ajbozarth left a comment

Choose a reason for hiding this comment

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

I have not yet reviewed the new FilterTools or Tags files, and have not played with the features locally yet, but here's a handful of review comments on current changes

props: IMetadataDisplayProps,
state: IMetadataDisplayState
): IMetadataDisplayState {
if (state.searchValue === '' && state.filterTags.length === 0) {
Copy link
Member

Choose a reason for hiding this comment

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

Echoing @kevin-bates comment, why are these if statements needed, it seems like we could remove this if block and the if wrapping around the next section and get the same results

Copy link
Member

@ajbozarth ajbozarth left a comment

Choose a reason for hiding this comment

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

A few comments on the new files, otherwise no big issues with them

@ajbozarth
Copy link
Member

UI/UX review comments:

  • Clicking the icon in a tag, the check mark or plus, doesn't "click" the tag as I would expect it to (in fact only the text in the tag is clickable)

  • Odd css happening with the down arrow icon:

Screen Shot 2020-11-03 at 12 20 42 PM

might be caused by using css instead of LabIcon
  • If you filter on tags then close the filter on tags section there is no way to determine that it's still filtering on tags, this may confuse users who filter then leave the snippet UI and come back a long time later

@marthacryan
Copy link
Member Author

image
Updated the "Filter by Tags" area to use a labicon of the caret up icon rather than the css it was using before. I added the behavior that when the "Filter by Tags" area is closed, it doesn't filter by any tags. I also changed the button to be completely "clickable", and not just the text.

It looks good except there's some weird css stuff going on in safari due to these changes - @ajbozarth maybe you could check it out? I'm not really finding anything to fix it (see below)
image

@ajbozarth
Copy link
Member

I've pushed a fix for the safari css you found.

As for the "carrot" icon under Filter By Tags, I think we should move it next to the text and have it swap between a up and down carrot depending on if the tags display is open, making it more clear that its a dropdown. Also I would suggest we use the carrot icons already in lab rather than adding our own.

As for the disabling of the filter when it's closed, I think a better implementation might be to let the user close it and still be filtered, but to highlight the Filter By Tags text to indicate whether filtering is being applied.

@ajbozarth
Copy link
Member

while testing my above fix I noticed some really odd behavior with the tags that should be further looked into as well, including a screen capture gif below to show it
Screen Recording 2020-11-05 at 4 04 11 PM

@marthacryan
Copy link
Member Author

If we're changing the design of the "filter by tags" button, what are your thoughts on having a tag button like the ToC uses? (see below)
image

@marthacryan
Copy link
Member Author

Here's the updated filter button:
tags

@marthacryan
Copy link
Member Author

Small update to the size of the gray box that appears on hover (makes it tighter around the icon):
image

@marthacryan
Copy link
Member Author

Fixed safari bug.

@marthacryan
Copy link
Member Author

image
Thoughts on this indicator for when filtering by tags and the tag area is closed?

Copy link
Member

@ajbozarth ajbozarth left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@lresende lresende left a comment

Choose a reason for hiding this comment

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

LGTM

@lresende lresende merged commit 63dd334 into elyra-ai:master Nov 9, 2020
lresende pushed a commit that referenced this pull request Nov 9, 2020
Add the ability to search/filter by tags to the metadata explorer.

Co-authored-by: Alex Bozarth <ajbozart@us.ibm.com>
Co-authored-by: Ai-Vy Dang <aivyndang@gmail.com>
Co-authored-by: Jay Ahn <aju960219@gmail.com>
Co-authored-by: Karla Spuldaro <karla.spuldaro@ibm.com>
Co-authored-by: Kiran Pinnipati <kpinnipati@gmail.com>
@marthacryan marthacryan deleted the add-search branch April 23, 2021 15:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Enable searching for code-snippets
6 participants