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

[Canvas] i18n work on workpad header (and a few header CTAs) and convert to typescript #44943

Merged
merged 11 commits into from
Sep 10, 2019

Conversation

poffdeluxe
Copy link
Contributor

@poffdeluxe poffdeluxe commented Sep 5, 2019

Summary

Converted WorkpadHeader to Typescript (with some adjustments to a few related components)

Adding i18n strings for the following components:

  • WorkpadHeader (subcomponents not yet complete)
  • Asset
  • AssetManager
  • AssetModal

slight modification to the embeddable flyout

Checklist

Use strikethroughs to remove checklist items you don't feel are applicable to this PR.

- [ ] This was checked for cross-browser compatibility, including a check against IE11
- [ ] Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support
- [ ] Documentation was added for features that require explanation or tutorials
- [ ] Unit or functional tests were updated or added to match the most common scenarios
- [ ] This was checked for keyboard-only and screenreader accessibility

For maintainers

@poffdeluxe poffdeluxe added review Project:i18n Team:Presentation Presentation Team for Dashboard, Input Controls, and Canvas loe:small Small Level of Effort v8.0.0 release_note:skip Skip the PR/issue when compiling release notes impact:high Addressing this issue will have a high level of impact on the quality/strength of our product. v7.5.0 labels Sep 5, 2019
@poffdeluxe poffdeluxe requested a review from a team as a code owner September 5, 2019 20:57
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-canvas

Copy link
Contributor

@clintandrewhall clintandrewhall left a comment

Choose a reason for hiding this comment

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

Nice work! Approving to unblock, but resolve the comments below before merging.

@@ -5,22 +5,40 @@
*/

import { connect } from 'react-redux';
import { Dispatch } from 'redux';
// @ts-ignore: Local Untyped
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: "Untyped local" - just so we can find them all later

setWriteable: (isWorkpadWriteable: boolean) => void;
}

const mapStateToProps = (state: any) => ({
Copy link
Contributor

Choose a reason for hiding this comment

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

State should be defined.

@@ -53,7 +76,7 @@ export class WorkpadHeader extends React.PureComponent {
</EuiToolTip>
);

_keyHandler = action => {
_keyHandler = (action: string) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

action appears to be a predictable type.

type Action = 'EDITING' | 'FULLSCREEN' | ...?

_keyHandler = (action: Action) => {

Might chat with @cqliu1 about where these are defined, then enumerate them there as a type. Otherwise, for now, just create a type here that matches what we're expecting.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Spoke with @cqliu1 about it.. seems like there's not a good way right now to grab all the actions. I looked briefly at defining them as a big union type but Catherine and I agreed it might be better to have them all listed as an enum that way you can reference them when adding to the keymap (not just referencing a magic string). Problem with doing that enum is that it might be a really not-fun refactor and I'm not sure we wanna block i18n work for it.

I went ahead and created this issue for it though: #45073

@@ -18,30 +19,52 @@ import {
EuiModalFooter,
EuiToolTip,
} from '@elastic/eui';

import { ComponentStrings } from '../../../i18n';
const { WorkpadHeader: strings } = ComponentStrings;
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: place const after all imports.

}),
getSpaceUsedText: (percentageUsed: number) =>
i18n.translate('xpack.canvas.assetModal.spacedUsedText', {
defaultMessage: `${percentageUsed}% space used`,
Copy link
Contributor

Choose a reason for hiding this comment

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

This should have a value, not use template strings:

defaultMessage: '{percentageUsed} space used',
values: {
  percentageUsed
}

https://github.com/elastic/kibana/blob/master/src/dev/i18n/README.md

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@poffdeluxe poffdeluxe force-pushed the i18n-canvas-workpad-header branch from 2747510 to 27eb09a Compare September 9, 2019 18:45
@elasticmachine
Copy link
Contributor

💔 Build Failed

@poffdeluxe
Copy link
Contributor Author

retest

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@poffdeluxe poffdeluxe merged commit b57824f into elastic:master Sep 10, 2019
@poffdeluxe poffdeluxe deleted the i18n-canvas-workpad-header branch September 10, 2019 14:30
jloleysens added a commit to jloleysens/kibana that referenced this pull request Sep 10, 2019
…-to-np-ready

* 'master' of github.com:elastic/kibana: (138 commits)
  [Canvas] i18n work on workpad header (and a few header CTAs) and convert to typescript (elastic#44943)
  update close/delete system index modals (elastic#45037)
  TS return type of createIndexPatternSelect (elastic#45107)
  [ML] Fix focus chart updating. (elastic#45146)
  [ML] Data frame transform: Fix progress in wizard create step. (elastic#45116)
  [Graph] Re-enable functional test (elastic#44683)
  [SIEM] unique table id for each top talkers table (elastic#45014)
  [SIEM] ip details heading draggable (elastic#45179)
  [Maps][File upload] Set complete on index pattern creation (elastic#44423)
  [Maps] unmount map embeddable component on destroy (elastic#45183)
  [SIEM] Adds error toasts to MapEmbeddable component (elastic#45088)
  fix redirect to maintain search query string (elastic#45184)
  [APM] One-line trace summary (elastic#44842)
  [Infra UI] Display non-metric details on Node Detail page (elastic#43551)
  [Maps][File upload] Removing bbox from parsed file pending upstream lib fix (elastic#45194)
  [Logs UI] Improve live streaming behavior when scrolling (elastic#44923)
  [APM] Fix indefinite loading state in agent settings for unauthorized user roles (elastic#44970)
  [Reporting] Rewrite addForceNowQuerystring to getFullUrls (elastic#44851)
  [Reporting/ESQueue] Improve logging of doc-update events (elastic#45077)
  [Reporting] Make screenshot capture less noisy by default (elastic#45185)
  ...
poffdeluxe added a commit to poffdeluxe/kibana that referenced this pull request Sep 11, 2019
…ert to typescript (elastic#44943)

* i18n work on workpad header and a few header ctas

* Convert WorkpadHeader to typescript

* String ordering cleanup

* Addressing some feedback

* Adding state

* lint

* Shortcut type refactor

* Revert "Shortcut type refactor"

This reverts commit d00e48853bcb16fdb14f9bca8b2536c920e8d650.

* Using new State type

* Removing unused type

* Updating state type
poffdeluxe added a commit that referenced this pull request Sep 12, 2019
…ert to typescript (#44943) (#45405)

* i18n work on workpad header and a few header ctas

* Convert WorkpadHeader to typescript

* String ordering cleanup

* Addressing some feedback

* Adding state

* lint

* Shortcut type refactor

* Revert "Shortcut type refactor"

This reverts commit d00e48853bcb16fdb14f9bca8b2536c920e8d650.

* Using new State type

* Removing unused type

* Updating state type
@poffdeluxe poffdeluxe restored the i18n-canvas-workpad-header branch September 12, 2019 15:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
impact:high Addressing this issue will have a high level of impact on the quality/strength of our product. loe:small Small Level of Effort Project:i18n release_note:skip Skip the PR/issue when compiling release notes review Team:Presentation Presentation Team for Dashboard, Input Controls, and Canvas v7.5.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants