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

Upgrade EUI to 13.4.1 #43717

Closed
wants to merge 11 commits into from
Closed

Upgrade EUI to 13.4.1 #43717

wants to merge 11 commits into from

Conversation

thompsongl
Copy link
Contributor

eui@13.3.0eui@13.4.1

Conversion of EuiSwitch to TypeScript and to use a button is the reason for most of the churn

13.4.1

  • Converted EuiSwitch to TypeScript (#2243)

Bug fixes

  • Added missing viewBox attribute to Docker, Kubernetes, and Redis logos (#2240)

13.4.0

  • Converted EuiFacetButton to TypeScript (#2226)
  • Adds an optional onClear prop to the the EuiDatePicker component (#2235)
  • Added support for onClick and href props on EuiListGroupItem and converted to TypeScript (#1933)

Bug fixes

  • Fixed EuiSwitch semantics to align with aria roles (#2193)
  • Removed Firefox's focus ring to match other browsers (#2193)
  • Added missing onChange TS defs for EuiRange (#2211)
  • Fixed EuiBadge text cursor to default pointer (#2234)
  • Fixed EuiPageContent className prop to allow the passed-in className to take cascade precedence over classes generated by the component (#2237)

@thompsongl thompsongl added chore v8.0.0 release_note:skip Skip the PR/issue when compiling release notes v7.4.0 labels Aug 21, 2019
@thompsongl thompsongl requested review from a team as code owners August 21, 2019 20:34
@legrego legrego self-requested a review August 21, 2019 20:38
Copy link
Contributor

@chandlerprall chandlerprall left a comment

Choose a reason for hiding this comment

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

Changes LGTM; package.json, TypeScript & snapshot changes are as expected from the EUI changelog

@@ -105,6 +105,7 @@ export class FeatureTable extends Component<Props, {}> {
aria-label={
checked ? `${record.feature.name} visible` : `${record.feature.name} disabled`
}
label=""
Copy link
Member

Choose a reason for hiding this comment

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

question It seems like we're using EuiSwitch incorrectly now. It looks like we'll be creating an empty label tag next to the switch now.

If we're ok using switches without a label (like our use case here), should we instead be creating the markup without the <label> tag, and making the label prop optional?

image

Copy link
Contributor Author

@thompsongl thompsongl Aug 21, 2019

Choose a reason for hiding this comment

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

Thanks, @legrego , I meant to call this out specifically.

Accessibility is the reason for the change (cc @myasonik), and I was going to run through the app and find the places with empty labels.

@snide, this seems like a design v. a11y situation, because now that I see it in use, the bare switch seems reasonable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@legrego We're going to roll back the changes to EuiSwitch. Not just because of your concerns, but also some more sticky upgrade migration things we've run into too close to FF.

I'll be closing this PR in favor of one with an altered scope and version.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for the heads-up, @thompsongl!

@elasticmachine
Copy link
Contributor

💔 Build Failed

@thompsongl thompsongl requested a review from snide August 22, 2019 14:18
@elasticmachine
Copy link
Contributor

💔 Build Failed

Copy link
Contributor

@joshdover joshdover left a comment

Choose a reason for hiding this comment

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

Platform changes LGTM

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@thompsongl
Copy link
Contributor Author

Closing this in favor of a forthcoming upgrade PR with an altered scope

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
chore release_note:skip Skip the PR/issue when compiling release notes v7.4.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants