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

Athena #126

Closed
10 of 11 tasks
tangjeff0 opened this issue Jun 4, 2020 · 19 comments
Closed
10 of 11 tasks

Athena #126

tangjeff0 opened this issue Jun 4, 2020 · 19 comments
Assignees

Comments

@tangjeff0
Copy link
Collaborator

tangjeff0 commented Jun 4, 2020

Tasks

  • typing bug Athena #126 (comment)
  • up and down to go through list
    • arrow keys work, but now needs the list needs to scroll
  • two items can be highlighted at once: one from mouse hover, one from arrow keys
  • enter - create new page or go to page
  • shift-enter - open in right sidebar
  • save and show recent searches Athena save and show recent items #208
  • debouncing
  • detect click outside of component to close
  • create keybinding event listener that opens Athena from anywhere (e.g. cmd-k)
  • UI part
  • search part First steps for search box #20

Design

Stuarts

@tangjeff0 tangjeff0 mentioned this issue Jun 8, 2020
5 tasks
This was referenced Jun 8, 2020
@prabhath6
Copy link
Contributor

Is anyone working on fixing the save and show recent searches task from the above list? If not I would like to pick it up.
Below is the gif from my local changes.

recent items

I have a few questions regarding the design of this feature that I would like to discuss before pushing the changes.

@tangjeff0
Copy link
Collaborator Author

tangjeff0 commented Jul 1, 2020

hi @prabhath6 thanks for this. I actually was just about to merge in some changes. I removed the cache, and added key down functionality (up, down, enter, shift-enter). This will likely affect how you implemented recent searches, but I'm happy to look at your fork.

@prabhath6
Copy link
Contributor

Hi @tangjeff0, you can find my changes below. We have modified the same functions so i expect merge conflicts.
I am not sure where to store the recent items, so for now i am saving them to the *recent-items atom. The content of the atom is reset when the page is refreshed.

@tangjeff0
Copy link
Collaborator Author

Awesome code @prabhath6 ! I just merged in #201, which removes the *cache and merges all state into one atom. It also puts search results from pages and blocks into one vector. It was easier to concat all those results together at query time, but we should probably still have a separate vector for recent items. In fact, it should probably be in the re-frame db as your comment asks.

@prabhath6
Copy link
Contributor

prabhath6 commented Jul 1, 2020

thanks @tangjeff0 🙂! I will pull in your changes, resolve the conflicts and move the recent-item to re-frame db.
One quick question

It also puts search results from pages and blocks into one vector.

All the search results or just the ones that the user clicked on.

@tangjeff0
Copy link
Collaborator Author

The vector is an (exact match OR nil), 20 page matches, and 20 block matches. If what you mean by the ones that the user clicked on are recents, it doesn't include those.

@tangjeff0
Copy link
Collaborator Author

It might also be easier to start with a fresh branch than to try to resolve the conflicts @prabhath6 .

@prabhath6
Copy link
Contributor

Hey @tangjeff0, i pulled in your changes and built the save and show feature on top of them. Now the state is stored in re-frame db instead of an atom. I have opened a pull request.

@nthd3gr33
Copy link
Contributor

Mani and Adrien, please comment here so I can assign you this task. Thanks 💚

@alaq
Copy link
Contributor

alaq commented Jul 8, 2020

👋 hi @nthd3gr33

  • up and down to go through list
    • arrow keys work, but now needs the list needs to scroll

Probably a detail but may I also suggest Ctrl+j and Ctrl+k, like on Roam?

@itsrainingmani
Copy link
Contributor

hi @nthd3gr33 🚀

@tangjeff0
Copy link
Collaborator Author

All keybindings will be configurable @alaq don't worry :)

@alaq
Copy link
Contributor

alaq commented Jul 17, 2020

  • ~ figure out why this doesn't work for Safari, Firefox, and probably other non-Chromium browsers
    • will this work with Tauri?

This seems to be working again (tested in Firefox and Safari).

However, we may have found another issue: with the test data loaded, if you type one character only in athena, the page freezes and you have to close it (tested in Chromium and Firefox).

@tangjeff0 let me know if you feel like this needs a separate issue.

@nthd3gr33
Copy link
Contributor

nthd3gr33 commented Jul 17, 2020

Quick update:

We (Team Seneca) made some progress on this today. We have implemented a solution so that the the arrow keys do scroll vertically through the menu items. That said, the animation is far from ideal. We also need to implement the following additional changes:

  • stop allowing the index to go lower than 0 (or -1?)
  • center the selected item vertically, except when the selected item is near the top or bottom of the list

Here is the code that we used today: (this goes in key-down-handler function in athena.cljs)

(= key KeyCodes.UP)
    (do
      (swap! state update :index dec)
      (let [cur-sel (first (array-seq (. js/document getElementsByClassName "selected")))]
        (.. cur-sel (scrollIntoView false {:behavior "smooth" :block "center"}))))

(= key KeyCodes.DOWN)
    (do
      (swap! state update :index inc)
      (let [cur-sel (first (array-seq (. js/document getElementsByClassName "selected")))]
        (.. cur-sel (scrollIntoView true {:behavior "smooth" :block "center"}))))

(code author: @itsrainingmani)

This issue of focus management turned out to be harder than we expected. My gut told me that we should do more research before hacking away further. Results of our research:

  • elem.focus() is not a usable solution due to a Firefox bug (I also read that this bug exists at times for Safari as well)
  • One of the most popular Reagent UI component libraries still has not finished Focus Management.
  • using tabindexed may be another solution that is worth pursuing. (see this SO post)
  • Roam uses blueprint as a React UI component library
  • We might be able to take inspiration from how blueprint implements some of this (see their select function)

We will be working more on this issue tomorrow.

@tangjeff0
Copy link
Collaborator Author

tangjeff0 commented Jul 17, 2020

It might be best to make two new issues: one for this typing bug and one for arrow-keys. @alaq @nthd3gr33 @itsrainingmani

@alaq
Copy link
Contributor

alaq commented Jul 17, 2020

Just opened #269 for the typing bug

@tangjeff0
Copy link
Collaborator Author

tangjeff0 commented Jul 17, 2020

Can you make a separate issue for arrow keys and copy this comment ?

@itsrainingmani
Copy link
Contributor

Opened issue for arrow keys #271

@nthd3gr33
Copy link
Contributor

nthd3gr33 commented Jul 19, 2020

The hover issue still remains. We implemented parity with Roam (aside from the flickering) as can be seen in the Loom, but the three of us are thinking that the Dynalist functionality is probably more ideal.

Any thoughts on which direction is best?

https://www.loom.com/share/6873e869c9b743cd92832953c86a569b

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

No branches or pull requests

5 participants