Skip to content
This repository has been archived by the owner on Jun 26, 2020. It is now read-only.

Add label for editor's toolbar #500

Merged
merged 19 commits into from
Aug 13, 2019
Merged

Add label for editor's toolbar #500

merged 19 commits into from
Aug 13, 2019

Conversation

msamsel
Copy link
Contributor

@msamsel msamsel commented May 20, 2019

Suggested merge commit message (convention)

Fix: Add label for editor's toolbar. Closes ckeditor/ckeditor5#1404.

BREAKING CHANGE: ToolbarView class will now have required locale argument, to translate the aria-label attribute of a toolbar element.

Additional information

Related PR: ckeditor/ckeditor5-widget#91

@coveralls
Copy link

coveralls commented May 20, 2019

Coverage Status

Coverage remained the same at 100.0% when pulling ce76a5a on t/ckeditor5/1404 into 1635784 on master.

@@ -131,6 +154,8 @@ export default class ToolbarView extends View {
this.focusTracker.remove( item.element );
} );

this.element.appendChild( this.labelView.element );
Copy link
Member

Choose a reason for hiding this comment

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

I don't think this is safe. In the same element we keep the collection of the items. In fact, the first thing that drew my attention was – why are you rendering the label separately and not in the base template.

Finally, do we need a separate element for that label? Can't we inline that label in the aria attribute?

@msamsel
Copy link
Contributor Author

msamsel commented Jul 4, 2019

I've updated this PR and made a static label for a toolbar. I also provide an option to override the aria-label, what is used by other view components which use ToolbarView class.

@oleq oleq self-requested a review July 19, 2019 09:37
lang/contexts.json Outdated Show resolved Hide resolved
lang/contexts.json Outdated Show resolved Hide resolved
@@ -123,13 +123,18 @@ export function createDropdown( locale, ButtonClass = DropdownButtonView ) {
* dropdown.render()
* document.body.appendChild( dropdown.element );
*
* **Please note:** {@link module:ui/toolbar/toolbarview~ToolbarView ToolbarView} instance created for this dropdown has an obligatory
Copy link
Member

Choose a reason for hiding this comment

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

Not sure we need this comment. A good example and solid API docs will do.

*/
constructor( locale ) {
constructor( locale, { ariaLabel } = {} ) {
Copy link
Member

Choose a reason for hiding this comment

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

Please, make this a property like ButtonView#label or ButtonView#tooltip with the default t( 'Editor toolbar' ).

]
],
role: 'toolbar',
'aria-label': ariaLabel
Copy link
Member

Choose a reason for hiding this comment

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

Bind to this.ariaLabel.

@@ -27,12 +27,23 @@ import { attachLinkToDocumentation } from '@ckeditor/ckeditor5-utils/src/ckedito
*/
export default class ToolbarView extends View {
/**
* @inheritDoc
* Creates an instance of the {@link module:ui/view~View} class.
Copy link
Member

Choose a reason for hiding this comment

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

That's not true.

* Also see {@link #render}.
*
* @param {module:utils/locale~Locale} locale The localization services instance.
* @param {Object} [config]
Copy link
Member

Choose a reason for hiding this comment

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

No config is needed. Properties are fine.

const toolbarView = dropdownView.toolbarView = new ToolbarView();
const locale = dropdownView.locale;
const t = locale.t;
const toolbarView = dropdownView.toolbarView = new ToolbarView( locale, { ariaLabel: t( 'Dropdown\'s toolbar' ) } );
Copy link
Member

Choose a reason for hiding this comment

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

Where are the tests?

@msamsel msamsel requested a review from oleq August 9, 2019 07:44
@Reinmar Reinmar removed their request for review August 12, 2019 12:09
@oleq oleq merged commit bdede90 into master Aug 13, 2019
@oleq oleq deleted the t/ckeditor5/1404 branch August 13, 2019 14:52
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Toolbar is incorrectly marked up
4 participants