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

Allowed for configuring editor label via config.label property #16792

Merged
merged 8 commits into from
Jul 31, 2024

Conversation

oleq
Copy link
Member

@oleq oleq commented Jul 24, 2024

Suggested merge commit message (convention)

Feature: Allowed to configure the accessible editable area label via config.label property. Closes #15208. Closes #11863. Closes #9731.

MINOR BREAKING CHANGE (ui): The default aria-label provided by InlineEditableUIView is now 'Rich Text Editor. Editing area: [root name]' (previously 'Editor editing area: [root name]'). Use the options.label constructor property to adjust the label.


Additional information

@oleq oleq changed the title Allowed for configuring editor title via config.title property Allowed for configuring editor title via config.label property Jul 24, 2024
@oleq oleq marked this pull request as ready for review July 24, 2024 14:59
@oleq oleq changed the title Allowed for configuring editor title via config.label property Allowed for configuring editor label via config.label property Jul 24, 2024
@oleq oleq requested a review from f1ames July 26, 2024 13:31
@f1ames f1ames self-assigned this Jul 29, 2024
Copy link
Contributor

@f1ames f1ames left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good 👍 Only some minor questions/suggestions from me.

[x] Q: What do we do about context config?

Can you elaborate? From what I understand, you suggest that label should be set with editor config and not context config, right? Should we document that somehow that setting label on context is not recommended because it may result in multiple editor instances with the same label? 🤔

@oleq
Copy link
Member Author

oleq commented Jul 31, 2024

Can you elaborate? From what I understand, you suggest that label should be set with editor config and not context config, right? Should we document that somehow that setting label on context is not recommended because it may result in multiple editor instances with the same label? 🤔

I'm going to use an example to explain. What I meant is that you can make the following changes to the manual test

diff --git a/packages/ckeditor5-comments/tests/manual/annotations-context.js b/packages/ckeditor5-comments/tests/manual/annotations-context.js
index f8f3dbcd5d..a9fd00d803 100644
--- a/packages/ckeditor5-comments/tests/manual/annotations-context.js
+++ b/packages/ckeditor5-comments/tests/manual/annotations-context.js
@@ -100,7 +100,8 @@ import InlineAnnotations from '../../src/annotations/inlineannotations.js';
                            editorConfig: {
                                     extraPlugins: [ Bold, Italic, Underline, List, Autoformat ]
                            }
-                  }
+                  },
+                  label: 'Context-wide label'
          } );
 
          const editor1 = await createEditor( document.querySelector( '.editors' ), context, channelId, 'editor1' );
@@ -241,7 +242,8 @@ async function createEditor( container, context, channelId, name ) {
                                     editorConfig: {
                                              extraPlugins: [ Bold, Italic, Underline, List ]
                                     }
-                           }
+                           },
+                           label: `Editor-level label: ${ name }`
                   } );
 
          const toggleEditorButton = document.createElement( 'button' );

and because of "label: Editor-level label: ${ name }" passed on the editor-level config, each editor will have a unique label. But if you remove this line, both will have "Context-wide label". So, as it turns out, there's no risk the context will force a specific label on all editors because this is still configurable.

@oleq oleq requested a review from f1ames July 31, 2024 11:35
Copy link
Contributor

@f1ames f1ames left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@oleq thanks for all the explanations and clarifications 👍

The rest of this PR and the code looks good too 🎉

@f1ames f1ames removed their assignment Jul 31, 2024
@oleq oleq merged commit b50ddbb into master Jul 31, 2024
7 checks passed
@oleq oleq deleted the ck/15208-editor-title branch July 31, 2024 12:51
@lnmp4000
Copy link

@oleq Could you provide some details of how this works with the multiroot editor. Is there some way of passing the aria-label configuration in as part of the call to addRoot?
I've hunted in the API docs, but I can't seem, to find anything. I thought something along the lines of:

            editor.addRoot(id, {
                data: getIn(getFieldMeta.current().value, id),
                isUndoable: false,
                ui: { ariaLabel: 'My Label' }
            });

@Witoso
Copy link
Member

Witoso commented Aug 30, 2024

This is scheduled for the next release, not published yet.

@lnmp4000
Copy link

lnmp4000 commented Sep 9, 2024

@lnmp4000 Check out https://ckeditor5.github.io/docs/nightly/ckeditor5/latest/api/module_core_editor_editorconfig-EditorConfig.html#member-label and https://ckeditor5.github.io/docs/nightly/ckeditor5/latest/api/module_editor-multi-root_multirooteditor-MultiRootEditor.html#function-createEditable.

@oleq Thanks for the link to that code. I probably have a fairly unique situation where I do not know what editable areas I will be attaching when I setup the mutliroot editor. So configuring labels within the create call is not possible.
Once the editor is configured, I later call addRoot to setup the editable areas. At this point I know the label, so is there a way of supplying config at this point?

e.g. at this point

            editor.addRoot(id, {
                data: getIn(getFieldMeta.current().value, id),
                isUndoable: false
            });

@oleq
Copy link
Member Author

oleq commented Sep 9, 2024

createEditable() accepts an accessible label as an argument. Please see the documentation of the addRoot() method to learn when to use it.

@lnmp4000
Copy link

lnmp4000 commented Sep 9, 2024

createEditable() accepts an accessible label as an argument. Please see the documentation of the addRoot() method to learn when to use it.

@oleq I'm not sure I have access to createEditable directly. I'm using @ckeditor/ckeditor5-react, which I think is complicating matters.

So the editor root is setup and the editor and editableElements (empty at this point) are loaded into a react context

 const { editableElements, toolbarElement, editor } = useMultiRootEditor({
   // -- config here -- //
})

Later I use the editor from the context to add the contenteditable element to the root.

  editor.addRoot(id, {
                data: getIn(getFieldMeta.current().value, id),
                isUndoable: false
            });

The editableElements updates which updates the context, and finally this is rendered inside my application along with the toolbarElement.

There is no direct call to createEditable I assume this is happening in the internals of the hook.

@oleq
Copy link
Member Author

oleq commented Sep 18, 2024

Thank you @lnmp4000 for a detailed explanation. The core editor API is ready for your use-case but the react integration isn't. Could you create a corresponding issue in https://github.com/ckeditor/ckeditor5-react/issues, please? Thank you.

@lnmp4000
Copy link

ckeditor/ckeditor5-react#529

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants