Conversation
artem-harbour
force-pushed
the
artem/naive-ui
branch
8 times, most recently
from
ae7f38a to
c953994
Compare
artem-harbour
requested review from
VladaHarbour,
caio-pizzol,
harbournick and
luccas-harbour
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c9539949f5
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
caio-pizzol
left a comment
caio-pizzol
left a comment
There was a problem hiding this comment.
@artem-harbour clean removal of naive-ui, nice work.
one bug: the overflow menu ("...") can't be closed by clicking outside or pressing Escape — only by clicking it again. see inline comment.
one small edge case on CommentsDropdown — also inline.
tests look good. left a few inline comments.
packages/superdoc/src/components/CommentsLayer/CommentsDropdown.vue
Outdated
Show resolved
Hide resolved
artem-harbour
force-pushed
the
artem/naive-ui
branch
2 times, most recently
from
c1caed9 to
07fa9d9
Compare
caio-pizzol
left a comment
caio-pizzol
left a comment
There was a problem hiding this comment.
@artem-harbour all five threads from last round are fixed, nice.
two small things still open — the file load error is now invisible to users, and dropdown menus don't close when clicking SVG icons. left inline comments.
also flagged the checkbox placeholder and a couple cleanup items. nothing blocking if the checkbox editing path is unused.
test coverage: the behavior spec is solid. ToolbarDropdown and CommentsDropdown could use unit tests for disabled state and option gating, but that's not blocking.
packages/super-editor/src/components/toolbar/ToolbarDropdown.vue
Outdated
Show resolved
Hide resolved
packages/superdoc/src/components/CommentsLayer/CommentsDropdown.vue
Outdated
Show resolved
Hide resolved
harbournick
left a comment
harbournick
left a comment
There was a problem hiding this comment.
Nice work @artem-harbour! Really clean. The one thing I noticed is that we used to be able to navigate dropdowns with the keyboard up/down arrows (seems to work in linked styles, but not in the others) and also select something just with 'enter'. This is important navigation functionality to retain. Otherwise looks great to me!
artem-harbour
force-pushed
the
artem/naive-ui
branch
from
07fa9d9 to
a3dcd26
Compare
caio-pizzol
left a comment
caio-pizzol
left a comment
There was a problem hiding this comment.
@artem-harbour the high-contrast dropdown styles got removed with naive-ui but weren't replaced — worth adding or tracking as a follow-up.
also flagged the dropdown copy-paste and a dead code branch — not blocking.
tests: click behavior is well covered. keyboard nav, CommentsDropdown, and the focus whitelist in PresentationEditor could use tests as a follow-up.
left a few inline comments.
| placement="bottom-start" | ||
| class="toolbar-button toolbar-dropdown sd-editor-toolbar-dropdown" | ||
| class="toolbar-button sd-editor-toolbar-dropdown" | ||
| :class="{ 'high-contrast': isHighContrastMode }" |
There was a problem hiding this comment.
the high-contrast class is still applied but the new dropdown has no styles for it. the old black-background-on-hover rules were removed with naive-ui and not replaced. high-contrast users now see the same light gray as everyone else. worth adding matching styles for the new dropdown?
| @@ -0,0 +1,465 @@ | |||
| <script setup> | |||
There was a problem hiding this comment.
ToolbarDropdown and CommentsDropdown copy-paste the same open/close/position/click-outside logic (~60 lines). worth pulling into a shared helper so bug fixes only happen once?
| const isSeparator = (item) => item.type === 'separator'; | ||
| const isOverflow = (item) => item.type === 'overflow'; | ||
|
|
||
| const getExpanded = (item) => { |
There was a problem hiding this comment.
item.expand is always a ref — the plain-value branch here never runs. safe to just use item.expand.value directly.
3 participants
Linear: SD-1190