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.

Sign up for GitHub

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

Jump to bottom

Added labels to buttons #42

Open
wants to merge 10 commits into
base: development_v2
Choose a base branch
from
Open

Added labels to buttons #42

wants to merge 10 commits into from

Conversation

NidhiKJha
Copy link
Contributor

@NidhiKJha NidhiKJha commented Jul 7, 2020 •
edited
Loading

This pull request is to solve the issue Form control element <button> has no associated label. This pull request is not complete. I am currently working on it to add more meaningful labels and tests.

jtoliver
jtoliver reviewed Jul 7, 2020
View reviewed changes
Copy link
Contributor

@jtoliver jtoliver left a comment

Choose a reason for hiding this comment

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

All the labels should be translated using i18n. They should be meaningful also. If the button is an icon of a trash can we should use the label "Remove". If the button is just text, use the button text. I think "menu" is fine for menus.

@NidhiKJha
Copy link
Contributor Author

@jtoliver I have added the translations to aria-label . Also, I couldn't see any translation for a toggle in buttons in .yml files. Do we need to add one?

@jtoliver
Copy link
Contributor

@NidhiKJha Make sure you pull the latest. All the buttons have been refactored to use the new action-button component. You should be able to set the aria-label a newly created (ariaLabel prop || text). Let me know if you have any questions

@NidhiKJha
Copy link
Contributor Author

NidhiKJha commented Aug 11, 2020 •
edited
Loading

Hi @jtoliver I have updated the code but I couldn't find ariaLabel prop here in primero/app/javascript/components/action-button/component.jsx. Also, I am not sure how we can use text as a prop. In primero/app/javascript/components/action-button/components/default-button/component.jsx we have used text={text}, is it for the button text or aria- label. Then in javascript/components/action-button/components/icon-button/component.jsx we have not used text here, but we have passed it in propTypes. Can you please help me understand this.

@jtoliver
Copy link
Contributor

jtoliver commented Aug 12, 2020 •
edited
Loading

You should be able to add the aria label to the Button component in components/action-button/components/default-button/component.jsx.

<Button
  className={clsx({
  ...
  aria-label="ariaLabel"
/>

The text prop there is the button text.

For javascript/components/action-button/components/icon-button/component.jsx it is just a button with an icon and no text. You can just add the aria label to the Fab component being rendered.

@NidhiKJha
Copy link
Contributor Author

Hi @jtoliver , I have made the suggested changes. Please review this pull request. Thanks 😄

jtoliver
jtoliver reviewed Aug 21, 2020
View reviewed changes
app/javascript/components/index-filters/components/filter-types/search.jsx Outdated
@@ -78,7 +78,7 @@ const Search = ({ handleReset }) => {
inputProps={{ "aria-label": i18n.t("navigation.search") }}
endAdornment={
<InputAdornment position="end">
<IconButton className={css.iconSearchButton} onClick={handleClear}>
<IconButton aria-label="Menu" className={css.iconSearchButton} onClick={handleClear}>
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you add i18n.t for Menu

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I couldn't find any direct translation for the menu in en.yml

app/javascript/components/nav/component.jsx Outdated
@@ -76,7 +76,7 @@ const Nav = () => {
<div className={css.drawerHeaderContainer}>
<Hidden mdUp implementation="css">
<div className={css.drawerHeader}>
<IconButton onClick={handleToggleDrawer(false)}>
<IconButton aria-label="Menu" onClick={handleToggleDrawer(false)}>
Copy link
Contributor

Choose a reason for hiding this comment

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

Here too

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I couldn't find any direct translation for the menu in en.yml

app/javascript/components/record-form/form/field-types/attachments/attachment-input.jsx Outdated Show resolved Hide resolved
app/javascript/components/record-form/nav/component.jsx Outdated Show resolved Hide resolved
app/javascript/components/record-list/record-list-toolbar/component.jsx Show resolved Hide resolved
@jtoliver
Copy link
Contributor

Left a few comments for you, make sure to run npm run lint also. Thanks

@NidhiKJha
Copy link
Contributor Author

Thanks for the review, I have made all the suggested changes but I couldn't find any direct translation for the menu in en.yml.

@NidhiKJha NidhiKJha requested a review from jtoliver August 21, 2020 15:18
@jtoliver-quoin
Copy link
Collaborator

You can add it under the buttons: key in /config/locales/en.yml. To generate a new locale file stop rails server then run rake primero:i18n_js, start rails server

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jtoliver jtoliver Awaiting requested review from jtoliver

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@NidhiKJha @jtoliver @jtoliver-quoin