[bug] Fixing search item in menu #6027

Phoenixton · 2019-03-11T22:33:59Z

What I did

Quickfix of issue #5772 , the search item in the menu bar is now working. Added an API function in lib/ui/src/core/layout.js that focuses on an element with the given id, if that element is focusable. There's room for an API that would handle focus (and probably other UI features) of more elements than what's right now available. Also, quick refactor via the created function.

tmeasday · 2019-03-11T23:21:42Z

@Phoenixton do you think maybe it would make sense to add an extra layer here? I don't love the fact that we ended up encoding the id of the search field in two different places.

Maybe

In layout.js - api.focusOnUIElement(..) + api.focusSearch() (which calls the former)
In shortcuts.js - search key handler that calls api.focusSearch in a setTimeout (to avoid event ordering problems).

PS -- should the code that toggles fullscreen / nav also be in the api.focusSearch() API? I guess it is not possible for the menu to be open unless we are in the right state already, but it feels like that doesn't really belong in the keyboard shortcut

Phoenixton · 2019-03-11T23:32:24Z

@tmeasday Thanks for the comment! Hmmm I agree that a better abstraction is needed, especially regarding the id, yeah. However, if I understand you correctly, your proposal means that if tomorrow we were to add another search bar (that wouldn't browse the stories but for example the addons, just for argument's sake), we would need another function in layout.js named for example api.focusSearchAddon() which would call focusOnUIElement(...) with the appropriate id. I'm not sure it's strictly better, although I hear you that hardcoding the ID twice is bad practice.

For your PS, I completely agree. I ended up thinking the same thing "anyway, if we're in the menu, we can't have fullScreen == true", but like you, I believe it's not in the best place it could be.

tmeasday · 2019-03-11T23:38:26Z

🤷‍♂️ We could also export a list of focus-able element ids somewhere?

Phoenixton · 2019-03-11T23:41:39Z

Yep, was my next suggestion. Sure seems like a nice and tidy way of handling it and maintaining the ids. And that way you keep a single neat function and .focus() checks for you if the element is focusable anyway, so no worries on that side. I like it, but I'm unsure of where it would be most appropriate to put that id list if we end up going that way. If we do, though, I can work on the ID refactoring at the same time.

tmeasday · 2019-03-11T23:43:35Z

There's probably a typescripty way to do it but i would probably just add api.focusableUIElements = {storySearch: '...'} in layout.js. What do you think?

Phoenixton · 2019-03-11T23:46:48Z

I need to check out Typescript so no idea on that part either, but yeah, either a focusableUIElements list in layout.js or an external file handling them, both are completely fine. To be honest, I was even going for a UIElements list, no extra need for the focusable part (as I said, .focus() handles the verification). I like having as little as possible hardcoded strings in my code, though, so that's me. We can try it first with the focusable list as you suggested!

tmeasday · 2019-03-11T23:49:25Z

Whatever you think is best is OK by me!

Phoenixton · 2019-03-11T23:49:56Z

On it 👍

codecov · 2019-03-12T00:53:17Z

Codecov Report

Merging #6027 into next will increase coverage by <.01%.
The diff coverage is 10%.

@@            Coverage Diff             @@
##             next    #6027      +/-   ##
==========================================
+ Coverage   34.92%   34.93%   +<.01%     
==========================================
  Files         648      648              
  Lines        9518     9519       +1     
  Branches     1352     1351       -1     
==========================================
+ Hits         3324     3325       +1     
- Misses       5577     5578       +1     
+ Partials      617      616       -1

Impacted Files	Coverage Δ
lib/ui/src/core/shortcuts.js	`20% <0%> (+1.18%)`	⬆️
lib/ui/src/containers/nav.js	`11.76% <0%> (-0.74%)`	⬇️
lib/ui/src/core/layout.js	`16.66% <16.66%> (ø)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 26a822a...bdfb720. Read the comment docs.

Phoenixton · 2019-03-12T00:55:32Z

@tmeasday If you wanna take a look, I added a temporary focusableUIElements array in layout.js

I'm not entirely sure that this should be here, tbh, but this was the best I could do off the top of my head to reduce the back and forth of variables. I'll take a closer look tomorrow but do tell me if you agree on principle.

I used the focusOnUIElement(...) function a bit more, too, which is reflected in the fact that you have a couple more ids in the list.

Edit: On a side note, we could attach a bit more info to the elements in the array. For example, in shortcuts.js, you've got

case 'focusIframe'

where instead of focusing the element you retrieve via the id, you focus on element.contentWindow
This could be added to the array via some kind of struct if we wanna abstract it further and handle everything via id.

tmeasday

Great stuff. LGTM!

I'm not really sure about the existing code; I didn't write it, I just moved it around! I think it is up for any kind of refactoring that makes sense.

Phoenixton · 2019-03-12T01:24:40Z

Nice, thanks a lot. I'll take a closer look tomorrow and open another PR if refactoring ends up being needed. Thanks again for your time :)

wuweiweiwu

Little changes. But LGTM

wuweiweiwu · 2019-03-12T08:13:00Z

lib/ui/src/core/layout.js

@@ -149,6 +156,17 @@ export default function({ store }) {
      });
    },

+    focusOnUIElement(elementId) {
+      const focusableElementId = focusableUIElements[elementId];
+      if (focusableElementId == null) {


nit: even if undefined == null can we change this check to be !focusableElements

Ah, it's what it was before, I was unsure of JS' best practices for that. Thanks!

wuweiweiwu · 2019-03-12T08:13:51Z

lib/ui/src/core/shortcuts.js

-          if (element) {
-            element.focus();
-          }
+          fullApi.focusOnUIElement('storyListMenu');


can you pull these constants out to be shared between modules?

Sure thing, on it. 👍

Yes, we'll change it to an enum later 👍

Hmmm then do you want me to just export them for now?

Phoenixton · 2019-03-12T09:32:40Z

@wuweiweiwu If you wanna take a look. Exporting the variables lets up refactor the test, I like it a bit better that way but if that wasn't what you had in mind, hit me up. 👍

wuweiweiwu · 2019-03-13T22:00:01Z

@Phoenixton looks awesome!

shilman · 2019-03-13T23:56:40Z

Awesome possum 💯 looks great 👏

[bug] Fixing search item in menu

PerrinePullicino added 2 commits March 11, 2019 22:54

adding an API function to focus on a document element

d7df1c1

fixing search item in menu and minor refactor

4f85dfe

Phoenixton requested review from Hypnosphi, igor-dv, ndelangen and tmeasday as code owners March 11, 2019 22:33

tmeasday added bug ui patch:yes Bugfix & documentation PR that need to be picked to main branch labels Mar 11, 2019

tmeasday added this to the 5.0.x milestone Mar 11, 2019

reformating focusable API and applying it to more elements

c4079e2

tmeasday approved these changes Mar 12, 2019

View reviewed changes

wuweiweiwu approved these changes Mar 12, 2019

View reviewed changes

exporting const list of focusable elements and moving test up

bdfb720

wuweiweiwu merged commit 01a9f24 into storybookjs:next Mar 13, 2019

shilman mentioned this pull request Mar 14, 2019

Search menu item broken #5772

Closed

shilman added the patch:done Patch/release PRs already cherry-picked to main/release branch label Mar 15, 2019

shilman pushed a commit that referenced this pull request Mar 15, 2019

Merge pull request #6027 from Phoenixton/fixing_search_item_in_menu

df69e18

[bug] Fixing search item in menu

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[bug] Fixing search item in menu #6027

[bug] Fixing search item in menu #6027

Phoenixton commented Mar 11, 2019

tmeasday commented Mar 11, 2019 •

edited

Loading

Phoenixton commented Mar 11, 2019

tmeasday commented Mar 11, 2019

Phoenixton commented Mar 11, 2019

tmeasday commented Mar 11, 2019 •

edited

Loading

Phoenixton commented Mar 11, 2019

tmeasday commented Mar 11, 2019

Phoenixton commented Mar 11, 2019

codecov bot commented Mar 12, 2019 •

edited

Loading

Phoenixton commented Mar 12, 2019 •

edited

Loading

tmeasday left a comment

Phoenixton commented Mar 12, 2019

wuweiweiwu left a comment

wuweiweiwu Mar 12, 2019

Phoenixton Mar 12, 2019

wuweiweiwu Mar 12, 2019

Phoenixton Mar 12, 2019

ndelangen Mar 12, 2019 •

edited

Loading

Phoenixton Mar 12, 2019

Phoenixton commented Mar 12, 2019

wuweiweiwu commented Mar 13, 2019

shilman commented Mar 13, 2019

[bug] Fixing search item in menu #6027

[bug] Fixing search item in menu #6027

Conversation

Phoenixton commented Mar 11, 2019

What I did

tmeasday commented Mar 11, 2019 • edited Loading

Phoenixton commented Mar 11, 2019

tmeasday commented Mar 11, 2019

Phoenixton commented Mar 11, 2019

tmeasday commented Mar 11, 2019 • edited Loading

Phoenixton commented Mar 11, 2019

tmeasday commented Mar 11, 2019

Phoenixton commented Mar 11, 2019

codecov bot commented Mar 12, 2019 • edited Loading

Codecov Report

Phoenixton commented Mar 12, 2019 • edited Loading

tmeasday left a comment

Choose a reason for hiding this comment

Phoenixton commented Mar 12, 2019

wuweiweiwu left a comment

Choose a reason for hiding this comment

wuweiweiwu Mar 12, 2019

Choose a reason for hiding this comment

Phoenixton Mar 12, 2019

Choose a reason for hiding this comment

wuweiweiwu Mar 12, 2019

Choose a reason for hiding this comment

Phoenixton Mar 12, 2019

Choose a reason for hiding this comment

ndelangen Mar 12, 2019 • edited Loading

Choose a reason for hiding this comment

Phoenixton Mar 12, 2019

Choose a reason for hiding this comment

Phoenixton commented Mar 12, 2019

wuweiweiwu commented Mar 13, 2019

shilman commented Mar 13, 2019

tmeasday commented Mar 11, 2019 •

edited

Loading

tmeasday commented Mar 11, 2019 •

edited

Loading

codecov bot commented Mar 12, 2019 •

edited

Loading

Phoenixton commented Mar 12, 2019 •

edited

Loading

ndelangen Mar 12, 2019 •

edited

Loading