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

feat: Recent Objects #6103

Merged
merged 66 commits into from
Jan 20, 2023
Merged

feat: Recent Objects #6103

merged 66 commits into from
Jan 20, 2023

Conversation

ozyx
Copy link
Member

@ozyx ozyx commented Jan 9, 2023

Closes #6053 #1646

Describe your changes:

Adds a "Recent Objects" draggable pane to the main left pane. Keeps track of the 20 most recently visited objects. Includes but distinguishes between objects and alias by displaying the objects' full paths and whether or not it is an alias. A target button next to each list item will scroll the object into view in the main tree. If an object or alias is deleted, its corresponding entries (and its children) in the Recent Objects list will also be removed.

Also adds the ability to persist the position of any Pane with a label attribute. Makes the left pane, inspector pane, and Recent Objects panes' positions persistent.

Screen.Recording.2023-01-09.at.9.32.14.AM.mov

TODO:

All Submissions:

  • Have you followed the guidelines in our Contributing document?
  • Have you checked to ensure there aren't other open Pull Requests for the same update/change?
  • Is this change backwards compatible? For example, developers won't need to change how they are calling the API or how they've extended core plugins such as Tables or Plots.

Author Checklist

  • Changes address original issue?
  • Tests included and/or updated with changes?
  • Command line build passes?
  • Has this been smoke tested?
  • Testing instructions included in associated issue OR is this a dependency/testcase change?

Reviewer Checklist

  • Changes appear to address issue?
  • Reviewer has tested changes by following the provided instructions?
  • Changes appear not to be breaking changes?
  • Appropriate automated tests included?
  • Code style and in-line documentation are appropriate?
  • Has associated issue been labelled unverified? (only applicable if this PR closes the issue)
  • Has associated issue been labelled bug? (only applicable if this PR is for a bug fix)

- Now throws an error if `getRelativeObjectPath` is passed a malformed `navigationPath`

- If the generated `objectPath` does not contain 'ROOT', add it

- Update some comments, fix some types

- Make `get` error message a bit more informative
- Adds the ability to pass in a custom objectPath to the ObjectPath component which will override the OriginalPath it displays by default
- Filter out deleted recents on re-render
- Registers/unregisters compositionCollection remove handlers for composable objects in RecentObjects.

- Removes items and their children from the RecentObjects list upon deletion.
@rukmini-bose rukmini-bose self-assigned this Jan 13, 2023
@ozyx ozyx requested a review from scottbell January 17, 2023 17:17
@ozyx ozyx marked this pull request as ready for review January 17, 2023 17:18
@ozyx
Copy link
Member Author

ozyx commented Jan 17, 2023

95% ready for review, have one failing e2e test to fix

@akhenry
Copy link
Contributor

akhenry commented Jan 17, 2023

Let's merge this and file a followup for vertical collapse and for the flashing highlight in the tree.

Copy link
Contributor

@scottbell scottbell left a comment

Choose a reason for hiding this comment

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

Looking good @ozyx! I had some review comments

src/ui/layout/RecentObjectsList.vue Show resolved Hide resolved
* and registers composition listeners for composable objects.
*/
getSavedRecentItems() {
const savedRecentsString = localStorage.getItem(LOCAL_STORAGE_KEY__RECENT_OBJECTS);
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we want to move any of this to the ObjectAPI?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good question. We save/load state to localStorage in the same way in mct-tree, so this is certainly mostly boilerplate that can be extracted. I've heard some discussion about revamping localStorage management, not sure if ObjectAPI would be the place though. @akhenry I'm deflecting to you, thoughts?

Either way, feels like something that would warrant its own separate issue/PR

Copy link
Contributor

Choose a reason for hiding this comment

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

Agree about a separate PR, was mainly curious about architecture here.

@scottbell
Copy link
Contributor

scottbell commented Jan 17, 2023

Is it supposed to be hidden by default?
image

It works well though! I like being able to drop recently viewed onto compositions - very handy.

Screen.Recording.2023-01-17.at.8.17.52.PM.mov

@ozyx
Copy link
Member Author

ozyx commented Jan 17, 2023

Is it supposed to be hidden by default? image

Yes, but this part (vertical pane collapse) is currently under construction

Copy link
Contributor

@davetsay davetsay left a comment

Choose a reason for hiding this comment

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

This looks awesome. I made some comments inline. It looks like you can ignore the vue linting props ones. That used to be a warning. Don't know what changed. Code is clean and well organized. Ideally I would like some of this logic, especially the shared logic with the other trees, extracted out of view/vue logic, but that's for another day.

Need to be fixed.

  • actions seem broken now (duplicate, move)

Should be fixed

  • doesn't react to non router changes (aka editing name from browse bar)

UI observations @charlesh88

  • same color highlight may be okay... but the recent items highlight doesn't go away which then makes it harder to distinguish what is selected and what is just highlighted
  • it is way more visibly clear and attention drawing than the tree itself. this might be a good thing. just pointing that out

src/api/objects/ObjectAPI.js Outdated Show resolved Hide resolved
src/api/objects/ObjectAPI.js Outdated Show resolved Hide resolved
src/ui/components/ObjectPath.vue Outdated Show resolved Hide resolved
src/ui/layout/Layout.vue Show resolved Hide resolved
src/ui/layout/RecentObjectsListItem.vue Outdated Show resolved Hide resolved
src/ui/layout/RecentObjectsListItem.vue Outdated Show resolved Hide resolved
src/ui/layout/RecentObjectsListItem.vue Outdated Show resolved Hide resolved
src/ui/layout/pane.vue Outdated Show resolved Hide resolved
@@ -390,9 +390,9 @@ class ApplicationRouter extends EventEmitter {
*
* @param {string} hash new hash for url
*/
hashChaged(hash) {
this.emit('change:hash', hash);
hashChanged(hash) {
Copy link
Contributor

Choose a reason for hiding this comment

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

👏 👏 👏

@ozyx ozyx requested a review from davetsay January 19, 2023 19:24
ozyx and others added 5 commits January 19, 2023 13:50
- Fixes an issue with calls to `router.navigate()`. It turns out that if the url passed in has query params in it, the navigation becomes unreliable. Will need further investigation, but for now I've updated the usages of that function and left a comment on the method specifying the format of the hash.
- min-height on vertical pane element.
- Font-size, margins tightened up and aligned.
@rukmini-bose rukmini-bose removed their assignment Jan 20, 2023
Copy link
Contributor

@davetsay davetsay left a comment

Choose a reason for hiding this comment

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

really nice job and you fixed all the comments super fast

@davetsay davetsay merged commit f98a2cd into master Jan 20, 2023
@davetsay davetsay deleted the mct6053-recent-objects-mark-ii branch January 20, 2023 18:27
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.

"Recently Viewed" items section for the left pane
6 participants