zestia / ember-modal-dialog

🔲 Animatable modal dialogs for Ember apps
MIT License
10 stars 2 forks source link

Don't focus the first element after the window gets focus #9

Closed azhiv closed 3 years ago

azhiv commented 3 years ago

So you have a special piece of code that focuses the first focusable element when the window gets focus. This behaviour is not consistent with the case of the initial dialog opening: In one of the dialogs of our app the first focusable element is ember-power-element trigger. So on the initial dialog open the form is neat, but if I change the browser tab and then change it back, the power calendar gets opened which is not desired. This happens because the focus on the trigger does this.

The best workaround I managed to find is overriding _handleFocusedWindow() {}, which is kinda ugly. Do you consider this feature to be absolutely necessary?

amk221 commented 3 years ago

Apologies, I added that to satisfy https://github.com/zestia/ember-modal-dialog/issues/7

In the thread, I even make your exact point

I wouldn't expect it to go to the first focusable element in the dialog. Because that is the choice of the developer

It obviously needs more thought!

Let me get back to you

amk221 commented 3 years ago

Hi, can you try this PR out: https://github.com/zestia/ember-modal-dialog/pull/10

azhiv commented 3 years ago

@amk221 I'm unable to yarn install neither of the two versions: 3.3.0, 3.3.1. The error I receive is: [1/4] 🔍 Resolving packages... error Couldn't find package "@zestia/animation-utils@^3.0.1" required by "@zestia/ember-modal-dialog@^3.3.0" on the "npm" registry.

Do you have an idea on how to resolve this?

amk221 commented 3 years ago

Sorry, I was suggesting checking out the repo itself, I've not published any code yet :)

Steps would be:

  1. Clone repo
  2. cd into it, run npm i npm link
  3. cd into your app run npm link @zestia/ember-modal-dialog
  4. run your app
azhiv commented 3 years ago

What I was trying to do is patching my package.json like so: "@zestia/ember-modal-dialog": "@zestia/ember-modal-dialog#_3.3.1", This is usually enough to point to a specific version of an addon. I'll try it your way though.

amk221 commented 3 years ago

Ok thanks for baring with me

azhiv commented 3 years ago

This is what I get:

 ~/src/ember-modal-dialog   _3.3.1  npm i
...
npm ERR! code E404
npm ERR! 404 Not Found - GET https://registry.npmjs.org/@zestia/animation-utils/-/animation-utils-3.0.0.tgz
npm ERR! 404
npm ERR! 404  '@zestia/animation-utils@3.0.0' is not in the npm registry.
npm ERR! 404 You should bug the author to publish it (or use the name yourself!)
npm ERR! 404 It was specified as a dependency of 'ember-modal-dialog'
azhiv commented 3 years ago

Should I also pull this addon and npm link it?

amk221 commented 3 years ago

Ok give that a shot please

azhiv commented 3 years ago

I succeeded building your branch! The input is not focused after I switch the tabs. However, is it supposed to keep the focus if the element had focus before the tab switching? Not a big deal, but it is something I would expect. https://user-images.githubusercontent.com/32125472/136362718-ffeeab4b-e652-4543-bd14-baef9d6ce7d6.mov

amk221 commented 3 years ago

Thanks for looking. I pushed a change to that branch, would you mind try it out?

azhiv commented 3 years ago

Looks good! I would proceed with this implementation.

amk221 commented 3 years ago

Released 3.3.2

azhiv commented 3 years ago

I'm sorry - have you published @zestia/animation-utils@^3.0.1? I'm getting this error when running yarn:

error Couldn't find package "@zestia/animation-utils@^3.0.1" required by "@zestia/ember-modal-dialog@^3.3.2" on the "npm" registry.
amk221 commented 3 years ago

👋 npm says it was published 2 days ago: https://www.npmjs.com/package/@zestia/animation-utils

azhiv commented 3 years ago

I see the modal-dialog but I can't find animation

amk221 commented 3 years ago

Aww crap, it's private.. woops. Let me sort that.

amk221 commented 3 years ago

Try now please!

azhiv commented 3 years ago
error @zestia/animation-utils@3.0.1: The engine "node" is incompatible with this module. Expected version ">= 14". Got "12.16.3"

We are using node 12.16.3. Can you loosen the requirement?

amk221 commented 3 years ago

Sure

amk221 commented 3 years ago

Done

azhiv commented 3 years ago

Great! Thanks for your efforts!