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

V8: Accessibility Changes For umbEditorHeader Directive (settings section) #6986

Merged
merged 5 commits into from
Dec 18, 2019
Merged

V8: Accessibility Changes For umbEditorHeader Directive (settings section) #6986

merged 5 commits into from
Dec 18, 2019

Conversation

RachBreeze
Copy link
Contributor

This PR extends #6315 adding the screen reader properties and setting the page title for almost all of the remaining menu items in the settings section. It leaves the language and the log viewer as the remaining items to update within this section

Rachel Breeze added 4 commits October 30, 2019 17:24
…ing the new header directive properties for screen reader only text, and for setting the page title
…the new header directive properties for screen reader only text, and for setting the page title
…menting the new header directive properties for screen reader only text, and for setting the page title
@poornimanayar
Copy link
Contributor

Hi @RachBreeze ,

Thank you, you know what happens next!

Poornima

Copy link
Contributor

@kjac kjac left a comment

Choose a reason for hiding this comment

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

@RachBreeze lovely 👍

A few comments from me:

  1. Could you please resolve the merge localization conflicts?
  2. Is there any reason why you're creating all those vm.header objects on the controllers? Wouldn't it be easier to simply supply the values directly in the editorfor and setpagetitle attributes of umb-editor-header?

@RachBreeze
Copy link
Contributor Author

RachBreeze commented Dec 16, 2019

Hi @kjac
Yep I can look at the merge conflicts.
I did try and set the properties on umb-editor-header originally but they wouldn't pass through to the header controller

…to v8/headerdirective-settings-section

# Conflicts:
#	src/Umbraco.Web.UI/Umbraco/config/lang/en.xml
#	src/Umbraco.Web.UI/Umbraco/config/lang/en_us.xml
@RachBreeze
Copy link
Contributor Author

Hi @kjac
Thank you once again for reviewing.
I've merged the language files, if you have a sec it would be good to know how I can set VS to keep the formatting the same as those in the xml files, even when I copy and paste the indentation I can't keep it the same :-(

Copy link
Contributor

@kjac kjac left a comment

Choose a reason for hiding this comment

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

@RachBreeze this works as advertised 👏 happy to approve it!

For future reference you can pass the values directly to the header directive like this:

<umb-editor-header
	name="vm.relationType.name"
	alias="vm.relationType.alias"
	hide-description="true"
	hide-icon="true"
	navigation="vm.page.navigation"
	editorfor="'relationType_tabRelationType'"
	setpagetitle="true">
</umb-editor-header>

...notice the single quotes around editorfor.

@kjac
Copy link
Contributor

kjac commented Dec 18, 2019

As for the language files, well... that's a longer story.

At some point in the past someone formatted one or more of them in a manner that doesn't conform to the default indentation which 4 spaces, as defined in .editorconfig:

# Default settings:
# A newline ending every file
# Use 4 spaces as indentation
[*]
insert_final_newline = true
end_of_line = crlf
indent_style = space
indent_size = 4

As a result, VS fails miserably at indenting these files.

What I usually do is hit Ctrl+Z immediately after copying and pasting an entry in the language files if the indentation looks wrong. VS will usually undo it's failed auto-formatting then, leaving behind something that at least looks correct (even if it is wrong according to .editorconfig).

Logic would dictate that we should simply force format those files in a separate commit to v8/dev and be done with it. Unfortunately this would leave a whole lot of existing PRs with a merge conflict 😞

@kjac kjac changed the base branch from v8/dev to v8/contrib December 18, 2019 13:08
@kjac kjac merged commit 578a725 into umbraco:v8/contrib Dec 18, 2019
@kjac
Copy link
Contributor

kjac commented Dec 18, 2019

Merged. Thanks again, @RachBreeze 👍

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

Successfully merging this pull request may close these issues.

4 participants