pod-os / PodOS

Personal Online Data Operating System
MIT License
13 stars 1 forks source link

refactor: split out pos-dialog #23

Closed josephguillaume closed 8 months ago

josephguillaume commented 1 year ago

Refactor to have dialog element with a common style

angelo-v commented 1 year ago

Great start! Looks good so far. I will have more time on wednesday but you safe to continue with the outlined todos

angelo-v commented 1 year ago

I also added a (general) Definition of Done you need to consider. Some of those things you already have on the list. Let me know if anything is unclear. Great work so far!

angelo-v commented 1 year ago

I hate do say it, but there is a bug in your solution: The dialog does not close when the form is submitted successfully. I could not figure out why yet, perhaps you can take a look and let me know if you need help with it.

There is also an issue with the focus: The first field (type) should be focused. At least on chrome this worked so far. I know about issues with firefox, but now it is not working in either of them.

angelo-v commented 1 year ago

I hate do say it, but there is a bug in your solution: The dialog does not close when the form is submitted successfully. I could not figure out why yet, perhaps you can take a look and let me know if you need help with it.

There is also an issue with the focus: The first field (type) should be focused. At least on chrome this worked so far. I know about issues with firefox, but now it is not working in either of them.

While extending the e2e to cover the closing behaviour I just got aware that closing currently also only works in chrome but not in firefox or webkit, so I guess your refactoring only broke in chrome what is not really working right now in general. Perhaps we can fix it by doing both focussing and closing explicitly with javascript instead of relying on the default dialog behaviour that seems to be at least inconsistent among browsers. Are you willing to look into this?

angelo-v commented 8 months ago

superseded by https://github.com/pod-os/PodOS/pull/32