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: add a close button to the modal #239

Merged
merged 2 commits into from
Sep 18, 2023
Merged

feat: add a close button to the modal #239

merged 2 commits into from
Sep 18, 2023

Conversation

AlphaGHX
Copy link
Contributor

image
给模态框右上角加个关闭按钮,顺便解决 iOS Safari 浏览器模态框占满屏不方便关闭的问题。
感觉可以给模态框封装个组件,毕竟有四个地方用到了。我现在对 solidjs 不太熟,说不定以后可以提个 pr。

@kunish
Copy link
Collaborator

kunish commented Sep 18, 2023

最近有点忙,没有时间好好 review,可以先用图标把 x 替换一下

@AlphaGHX
Copy link
Contributor Author

最近有点忙,没有时间好好 review,可以先用图标把 x 替换一下

好的,这也是个小问题,大佬慢慢 review。
好像 iOS 17 Safari 有点毛病来的。打开模态框自动 focus 第一个表单元素 😂

@kunish
Copy link
Collaborator

kunish commented Sep 18, 2023

最近有点忙,没有时间好好 review,可以先用图标把 x 替换一下

好的,这也是个小问题,大佬慢慢 review。 好像 iOS 17 Safari 有点毛病来的。打开模态框自动 focus 第一个表单元素 😂

🥺 您是大佬您是大佬,刚上线完,我再看一下哈

了解了,safari 有些时候可能是为了 a11y 做的优化,但是某些场景下反而是影响用户体验的

没事,这个咱们后面再看,如果大佬有兴趣继续跟进模态框相关的实现,欢迎随时 PR (但是可能为了日后好维护,我可能会提一些建议和意见哈,希望不要介意,咱们可以共同讨论)

@kunish
Copy link
Collaborator

kunish commented Sep 18, 2023

看起来 PR 蛮不错的,质量挺高,给大佬点个赞

有一个疑问是,这个顶部的 action 区域有必要设置一个大于默认 z-index 的值么

@AlphaGHX
Copy link
Contributor Author

image
上图为 z-index 为 0 的时候可能出现的问题(较难复现),而我希望这个按钮再下滑后依旧能显示在最顶层,所以这样处理了。

image
对于这个 PR 其实我有更多想法,如果顶部这么大块空间只是在右侧放个按钮是不是有点太空了,所以我考虑要不要像这样再左侧加点东西,如上图。
image
如果这样处理,我就还需要花点时间处理重叠的问题。

@kunish
Copy link
Collaborator

kunish commented Sep 18, 2023

OK,理解了,没问题

这个 PR 先合并,之后按你想法处理

非常感谢 🤗

@kunish kunish merged commit 737f1b0 into MetaCubeX:main Sep 18, 2023
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.

None yet

2 participants