Conversation
|
Hello, I'm the AEM Code Sync Bot and I will run some actions to deploy your branch.
|
hannessolo
requested review from
anfibiacreativa,
kozmaadrian and
sharanyavinod
| } | ||
|
|
||
| function everyTextblockInSelection(state, pred) { | ||
| let seen = false; |
There was a problem hiding this comment.
Do we want to establish a convention for booleans naming like
isVisible
isOk
etc?
cc: @sharanyavinod
There was a problem hiding this comment.
For locally scoped variables IMO we don't need this. But happy to be convinced otherwise. It would probably trigger renaming a lot of things though.
There was a problem hiding this comment.
we don't need it locally, but it's a good way to consolidate and make code faster to understand.
We can start enforcing for other cases, though, and leave a big refactor as a TODO
There was a problem hiding this comment.
I'm not convinced. I think "seen" and "ok" are already pretty obviously booleans (eg. response.ok is a really well known property in the fetch api). And anyway it would have to be hasSeen , not isSeen, in this case, because the function has seen it, not the function is being seen.
There was a problem hiding this comment.
oh I was just giving examples, this is why I mentioned isVisible, not isSeen. But I get your point. Ok, is ok (pun intended) because it maps directly to a response.ok for a 200/299 range. Not sure seen is very intuitive I would call it hasTextBlocks or isNotEmpty.
This is not a blocker to merge the PR, but it would be great to find our standards to work faster and have a style reference.
| 'bullet-list': (state, dispatch) => wrapInList(state.schema.nodes.bullet_list)(state, dispatch), | ||
| 'numbered-list': (state, dispatch) => wrapInList(state.schema.nodes.ordered_list)(state, dispatch), | ||
| 'heading-1': (s, d) => applyHeadingLevel(s, d, 1), | ||
| 'heading-2': (s, d) => applyHeadingLevel(s, d, 2), |
There was a problem hiding this comment.
Curious about the change from set to apply. Is it because it's not strictly a setter with a getter?
There was a problem hiding this comment.
It's just because I wanted a unified way of handling this between the slash menu and the toolbar. I think "apply" is more what we're doing than "set".
In the previous iteration of the code, it wasn't even consistently "set", I don't think setBulletList makes sense as a name
| getToken, | ||
| }); | ||
| path: controllerPathnameFromEditorCtx(this.ctx), | ||
| getToken: createQuickEditGetToken(), |
There was a problem hiding this comment.
Couldn't this be just a generic utility getToken function?
There was a problem hiding this comment.
Yes, let me fix that. Please do call out more pointless functions like that. I had cursor refactor but it created a lot of wrapper functions that don't add value, I should have used a better model...
There was a problem hiding this comment.
I am somewhat convinced that we could make other functions in that same file that could be generic with more config tradeoff. But this one stood out.
sharanyavinod
left a comment
sharanyavinod
left a comment
There was a problem hiding this comment.
We need a single source of truth for command config. The same information (id, schema name, label, icon etc) is currently spread across the code. Adding or renaming a command requires changes in multiple locations, and there's no overarching view of what's supported.
|
FYI: I see you've been adding icons into https://github.com/adobe/da-nx/tree/ew/nx2/blocks/img/icons whereas we've been adding into https://github.com/adobe/da-nx/tree/ew/nx2/img/icons. Note for when you're moving things into da-live. |
3 participants
#256
https://ew-toolbar--da-nx--adobe.aem.page
https://da.live/canvas?nx=ew-toolbar&quick-edit=ew-toolbar&nxver=2#/exp-workspace/frescopa/index