Skip to content
This repository has been archived by the owner on May 22, 2024. It is now read-only.

Action header - Add AriaLabel Text #50

Merged
merged 15 commits into from
Jun 6, 2017

Conversation

jmsv6d
Copy link
Contributor

@jmsv6d jmsv6d commented May 17, 2017

Summary

Adding aria-label text for the icons on the buttons in the action header.

Thanks for contributing to Terra.
@cerner/terra

@@ -104,6 +114,11 @@ var propTypes = {
onPrevious: _propTypes2.default.func,

/**
* Locale for i18n of accessability text.
**/
locale: _propTypes2.default.string,
Copy link
Contributor

Choose a reason for hiding this comment

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

Should setting locale be at the application level?

Copy link
Contributor

Choose a reason for hiding this comment

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

},
"peerDependencies": {
"react": "15.4.2",
"react-dom": "15.4.2",
"react-intl": "^2.3.0",
Copy link
Contributor

Choose a reason for hiding this comment

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

We need this package?

@@ -69,108 +75,148 @@ const defaultProps = {
onNext: null,
onPrevious: null,
children: null,
locale: 'en-US',
Copy link
Contributor

@StephenEsser StephenEsser May 22, 2017

Choose a reason for hiding this comment

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

I'd talk to some of the i18n people, but I thought I remembered someone saying we shouldn't default locale's in one of our meeting, if that turns out to be the case, I'd probably mark the locale as required and remove the default

Copy link
Contributor

Choose a reason for hiding this comment

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

Is the locale handled by the release of terra-base v0.5.0?

@mjhenkes
Copy link
Contributor

mjhenkes commented Jun 2, 2017

@jmsv6d are there changes you still need to make on this PR?

@@ -0,0 +1,243 @@
'use strict';
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Deleted file

.gitignore Outdated
@@ -51,3 +51,4 @@ reports
screenshots
target
lib
**/aggregated-translations/*.*
Copy link
Contributor

Choose a reason for hiding this comment

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

If we're adding this to .gitignore, we should also delete the folder itself.

@@ -71,6 +76,15 @@ const defaultProps = {
children: null,
};

const contextTypes = {
Copy link
Contributor

Choose a reason for hiding this comment

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

I am still curious, with adding contextTypes, do we still need to pass in the locale prop.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ooops, no. Removed the locale prop.

@mjhenkes mjhenkes temporarily deployed to terra-clinical-deployed-pr-50 June 6, 2017 14:15 Inactive
@mjhenkes mjhenkes temporarily deployed to terra-clinical-deployed-pr-50 June 6, 2017 14:22 Inactive
@ryanthemanuel ryanthemanuel merged commit cf70b6b into master Jun 6, 2017
@ryanthemanuel ryanthemanuel deleted the ActionHeader-AddFallbackTextToIcons branch June 6, 2017 16:16
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.

None yet

7 participants