Skip to content

fix(NotificationDrawer): add Translation capability - #339

Merged
jeff-phillips-18 merged 1 commit into
patternfly:masterfrom
glekner:fix-notification-translation
May 14, 2018
Merged

fix(NotificationDrawer): add Translation capability#339
jeff-phillips-18 merged 1 commit into
patternfly:masterfrom
glekner:fix-notification-translation

Conversation

@glekner

@glekner glekner commented May 10, 2018

Copy link
Copy Markdown
Contributor

affects: patternfly-react

What:
Added ability to add Translations to the Drawer. Components that can be translated:

  translations: {
    title: 'Notifications',
    emptyState: 'No Notifications Available',
    readAll: 'Mark All Read',
    clearAll: 'Clear All'
  }

Panels and Notifications were already translatable.

@glekner
glekner force-pushed the fix-notification-translation branch from 3b6dcbf to 13ab649 Compare May 10, 2018 12:25
cdcabrera
cdcabrera reviewed May 10, 2018
View reviewed changes

@cdcabrera cdcabrera left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Minor question, and of course a rebase.

Comment thread packages/core/src/components/Notification/Wrappers/NotificationDrawerPanelWrapper.js
>
<Button bsStyle="link" onClick={() => onMarkPanelAsRead(panelkey)}>
Mark All Read
{translations.readAll}

@cdcabrera cdcabrera May 10, 2018 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@gilad215 just curious what happens when someone accidentally replaces the entire translations object instead of merging or Object.assigning it, or is that even a concern? If we are concerned, could a possible solution involve placing checks for the translation object property, or maybe they're, or it is, required?

@glekner glekner May 10, 2018 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

translations is not a required prop.
I don't see how someone can accidentally override the default object without actually writing
translations={...}
If someone makes mistakes, thats their responsibility to fix it, because the object's properties are documented in the storybook

@cdcabrera cdcabrera May 10, 2018 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

For clarity, if a consumer only handles these properties ...

translations: {
  emptyState: 'Lorem Ipsum Available',
  readAll: 'Lorem Ipsum Read',
  clearAll: 'Lorem Ipsum Clear'
}

What's the expected outcome?

@glekner glekner May 10, 2018 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The title will be empty, Do you think consumers will want to translate only some of the properties? seems weird to me. If they do it by mistake i'm pretty sure they will understand why. Also, having a prop for each component doesn't look like a solution either, this is a very rare case..

cdcabrera
cdcabrera previously approved these changes May 10, 2018
View reviewed changes

@cdcabrera cdcabrera left a comment •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thinking it's always good to acknowledge limitations @gilad215

There are consumers and use cases that aren't always obvious. Your implementation is direct and works, nothing wrong with that. We can always re-approach at a later time if it pops back.

Alternatives possibly could have included something like using objectOf or shape, but that may have brought with it other outcomes

@glekner
glekner dismissed stale reviews from cdcabrera and jeff-phillips-18 via fd12b48 May 10, 2018 21:53
@glekner
glekner force-pushed the fix-notification-translation branch from 13ab649 to fd12b48 Compare May 10, 2018 21:53
jeff-phillips-18
jeff-phillips-18 previously approved these changes May 11, 2018
View reviewed changes

@jeff-phillips-18 jeff-phillips-18 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'd say this is OK as is, though I agree with @cdcabrera that we may want to revisit to define the required strings so they can be well known (shown in storybook) and/or handle defaulting those which are not set.

@glekner
glekner force-pushed the fix-notification-translation branch from fd12b48 to 8936f35 Compare May 13, 2018 11:38
@glekner

glekner commented May 13, 2018 •
edited
Loading

Copy link
Copy Markdown
Contributor Author
  • Added Shape Proptype, Spread wrapper so consumers can now translate only the properties they want to @cdcabrera
// Inside NotificationDrawerWrapper
  const translationsWrapper = {
    ...NotificationDrawerPanelWrapper.defaultProps.translations,
    ...translations
  };
  • Added more translatable components : unreadEvent, unreadEvents, deleteNotification
  • Added NotificationDrawerEmptyState with a default english title

@glekner
glekner force-pushed the fix-notification-translation branch from 8936f35 to 1a928be Compare May 13, 2018 11:55
@patternfly patternfly deleted a comment from coveralls May 13, 2018
@glekner
glekner force-pushed the fix-notification-translation branch from 1a928be to 5fb3c50 Compare May 14, 2018 09:04
@glekner
glekner force-pushed the fix-notification-translation branch from 5fb3c50 to 05f0bd9 Compare May 14, 2018 09:07
cdcabrera
cdcabrera approved these changes May 14, 2018
View reviewed changes

@cdcabrera cdcabrera left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Awesome! looks good @gilad215

@jeff-phillips-18
jeff-phillips-18 merged commit 27b206e into patternfly:master May 14, 2018
@glekner
glekner deleted the fix-notification-translation branch May 14, 2018 14:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@cdcabrera cdcabrera cdcabrera approved these changes
@jeff-phillips-18 jeff-phillips-18 jeff-phillips-18 approved these changes

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

@glekner @cdcabrera @jeff-phillips-18