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

[kbnTopNav] break out controller, allow implementors to pass it in #6752

Merged
merged 2 commits into from
Apr 4, 2016
Merged

[kbnTopNav] break out controller, allow implementors to pass it in #6752

merged 2 commits into from
Apr 4, 2016

Conversation

spalger
Copy link
Contributor

@spalger spalger commented Apr 2, 2016

The first iteration of kbnTopNav prevented the need to define a configTemplate and the menuItems in separate locations, but moved the external API to a kbnTopNav object that was exposed on the scope where kbnTopNav was rendered. This meant that in order to programmatically control the kbnTopNav other directives needed to share a scope with it, or inherit from its scope. Additionally, since the kbnTopNav object is potentially not available when other directives boot, all access to this object needed to be wrapped in a call to $timeout.

In order to prevent this, but keep the nice API of kbnTopNav, this change breaks the controller and its API out of kbnTopNav and exposes it via a Private module. Now, implementors can instantiate the KbnTopNavController themselves and pass it into the <kbn-top-nav> directive via the config attribute. This allows implementors to store the topNav API wherever makes sense in their implementation.

When the config attribute is not already an instance of the KbnTopNavController the directive will create an instance out of the passed value, which means that no existing implementations need to change at all.

@spalger
[kbnTopNav] break out controller, allow implementors to pass it in
9924a58 
The first iteration of `kbnTopNav` prevented the need to define a `configTemplate` and the `menuItems` in separate locations, but moved the external API to a `kbnTopNav` object that was exposed on the scope where `kbnTopNav` was rendered. This meant that in order to programmatically control the `kbnTopNav` other directives needed to share a scope with it, or inherit from its scope. Additionally, since the `kbnTopNav` object is potentially not available when other directives boot, all access to this object needed to be wrapped in a call to `$timeout`.

In order to prevent this, but keep the nice API of `kbnTopNav`, this change breaks the controller and it's API out of `kbnTopNav` and exposes it via a Private module. Now, implementors can instantiate the `KbnTopNavController` themselves and pass it into the `<kbn-top-nav>` directive via the config attribute. This allows implementors to store the topNav API wherever makes sense in their implementation.

When the config attribute is not already an instance of the `KbnTopNavController` the directive will create an instance out of the passed value, which means that no existing implementations need to change at all.
@spalger
[kbnTopNav] expand the tests to verify 9924a58
ded3e79
@panda01
Copy link
Contributor

panda01 commented Apr 4, 2016

Hmm, I fail to see how this solves the problem of not being being able to open something on default. I like the tests, they're comprehensive though.

@panda01 panda01 assigned spalger and unassigned panda01 Apr 4, 2016
@spalger
Copy link
Contributor Author

spalger commented Apr 4, 2016

Perhaps e46dfe7, where I implemented the kbnTopNavController into the upcoming console app, better illustrates the benefits?

The gist of it though is that it allows external code to control the kbnTopNav by interacting directly with it's controller. The way to do that without $timeouts or $scope traversal is to pass a value (the controller) down from a higher scope, so here all of the logic was pulled out of the kbnTopNav and into the KbnTopNavController, and the kbnTopNav adjusted to create it's controller via KbnTopNavController or accept an external one via $scope.

@spalger spalger assigned panda01 and unassigned spalger Apr 4, 2016
@panda01
Copy link
Contributor

panda01 commented Apr 4, 2016

Ah, silly me I missed this line https://github.com/elastic/kibana/pull/6752/files#diff-2c224dd6da43daa1ed57d4f297288512R10.

This is a nice extension of usability on the kbnTopNav.

LGTM!

@panda01 panda01 assigned spalger and unassigned panda01 Apr 4, 2016
@panda01
Copy link
Contributor

panda01 commented Apr 4, 2016

Maybe, this is eligible to have a second set of eyes on this.

@spalger spalger assigned lukasolson and unassigned spalger Apr 4, 2016
@rashidkpc
Copy link
Contributor

Definitely want a 2nd set of eyes on this

@rashidkpc
Copy link
Contributor

Can you separate the rename from the changes to make the diff easier to read?

@spalger
Copy link
Contributor Author

spalger commented Apr 4, 2016

This branch is already merged into others, so I feel like a rebase is a lot more work than comparing the two files side by side.

@spalger
Copy link
Contributor Author

spalger commented Apr 4, 2016

I broke out the changes in a separate branch so that we can see them in a different light, and they match the current branch exactly, but like I mentioned earlier it would be best if I could leave this history the way it is

Commits are split by change types:

@lukasolson
Copy link
Member

LGTM

@lukasolson lukasolson assigned spalger and unassigned lukasolson Apr 4, 2016
@spalger spalger merged commit 9ffa6a2 into elastic:master Apr 4, 2016
@spalger spalger deleted the implement/kbnTopNavController branch October 18, 2019 15:50
jbudz pushed a commit that referenced this pull request May 15, 2023
@cee-chen @kibanamachine
Upgrade EUI to v79.0.1 (#156925)
b9ea4ce 
## Summary

`eui@77.2.2` ⏩ `eui@79.0.1`

🦴 The primary changes in this upgrade are around the deprecated
`EuiLoadingContent` being removed in favor of `EuiSkeletonText`.
- Most instances have been a [direct swap of
usage](327626a),
but [some replacements were a bit more
opinionated](e6ceb36)
as I saw them as potential to take advantage of `EuiSkeletonText`'s
syntactical sugar and screen reader announcements for when state
switches to loaded.

---

## [`79.0.1`](https://github.com/elastic/eui/tree/v79.0.1)

**Bug fixes**

- Fixed broken push `EuiFlyout` behavior
([#6764](elastic/eui#6764))


## [`79.0.0`](https://github.com/elastic/eui/tree/v79.0.0)

- Updated all `EuiSkeleton` components with new props that allow for
more control over screen reader live announcements:
`announceLoadingStatus`, `announceLoadedStatus`, and `ariaLiveProps`
([#6752](elastic/eui#6752))
- Improved keyboard accessibility in `EuiPageHeader` by ensuring the
right side menu items come into focus from left to right.
([#6753](elastic/eui#6753))

**Breaking changes**

- Removed deprecated `EuiLoadingContent`. Use the `EuiSkeleton`
components instead. ([#6754](elastic/eui#6754))


## [`78.0.0`](https://github.com/elastic/eui/tree/v78.0.0)

- Improved the contrast ratio of `EuiCheckbox`, `EuiRadio`, and
`EuiSwitch` in their unchecked states to meet WCAG AA guidelines.
([#6729](elastic/eui#6729))
- Added React Testing Library `*ByTestSubject` custom commands to
`within()`. RTL utilities can be imported from
`@elastic/eui/lib/test/rtl`.
([#6737](elastic/eui#6737))
- Updated `EuiAvatar` to support a new letter `casing` prop that allow
customizing text capitalization
([#6739](elastic/eui#6739))
- Updated `EuiFocusTrap` to support the `gapMode` prop configuration
(now defaults to `padding`)
([#6744](elastic/eui#6744))

**Bug fixes**

- Fixed inconsistency in `EuiSearchBar`'s AND/OR semantics between DSL
and query string generation
([#6717](elastic/eui#6717))
- Fixed `EuiFieldNumber`'s native browser validity detection causing
extra unnecessary rerenders
([#6741](elastic/eui#6741))
- Fixed the `scrollLock` property on `EuiFocusTrap` (and other
components using `EuiFocusTrap`, such as `EuiFlyout` and `EuiModal`) to
no longer block scrolling on nested portalled content, such as combobox
dropdowns ([#6744](elastic/eui#6744))

**Breaking changes**

- `EuiAvatar`s with the default `user` type will now default to
capitalizing all initials in uppercase
([#6739](elastic/eui#6739))

---------

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
jasonrhodes pushed a commit that referenced this pull request May 17, 2023
@cee-chen @kibanamachine @jasonrhodes
Upgrade EUI to v79.0.1 (#156925)
14c8acc 
## Summary

`eui@77.2.2` ⏩ `eui@79.0.1`

🦴 The primary changes in this upgrade are around the deprecated
`EuiLoadingContent` being removed in favor of `EuiSkeletonText`.
- Most instances have been a [direct swap of
usage](327626a),
but [some replacements were a bit more
opinionated](e6ceb36)
as I saw them as potential to take advantage of `EuiSkeletonText`'s
syntactical sugar and screen reader announcements for when state
switches to loaded.

---

## [`79.0.1`](https://github.com/elastic/eui/tree/v79.0.1)

**Bug fixes**

- Fixed broken push `EuiFlyout` behavior
([#6764](elastic/eui#6764))


## [`79.0.0`](https://github.com/elastic/eui/tree/v79.0.0)

- Updated all `EuiSkeleton` components with new props that allow for
more control over screen reader live announcements:
`announceLoadingStatus`, `announceLoadedStatus`, and `ariaLiveProps`
([#6752](elastic/eui#6752))
- Improved keyboard accessibility in `EuiPageHeader` by ensuring the
right side menu items come into focus from left to right.
([#6753](elastic/eui#6753))

**Breaking changes**

- Removed deprecated `EuiLoadingContent`. Use the `EuiSkeleton`
components instead. ([#6754](elastic/eui#6754))


## [`78.0.0`](https://github.com/elastic/eui/tree/v78.0.0)

- Improved the contrast ratio of `EuiCheckbox`, `EuiRadio`, and
`EuiSwitch` in their unchecked states to meet WCAG AA guidelines.
([#6729](elastic/eui#6729))
- Added React Testing Library `*ByTestSubject` custom commands to
`within()`. RTL utilities can be imported from
`@elastic/eui/lib/test/rtl`.
([#6737](elastic/eui#6737))
- Updated `EuiAvatar` to support a new letter `casing` prop that allow
customizing text capitalization
([#6739](elastic/eui#6739))
- Updated `EuiFocusTrap` to support the `gapMode` prop configuration
(now defaults to `padding`)
([#6744](elastic/eui#6744))

**Bug fixes**

- Fixed inconsistency in `EuiSearchBar`'s AND/OR semantics between DSL
and query string generation
([#6717](elastic/eui#6717))
- Fixed `EuiFieldNumber`'s native browser validity detection causing
extra unnecessary rerenders
([#6741](elastic/eui#6741))
- Fixed the `scrollLock` property on `EuiFocusTrap` (and other
components using `EuiFocusTrap`, such as `EuiFlyout` and `EuiModal`) to
no longer block scrolling on nested portalled content, such as combobox
dropdowns ([#6744](elastic/eui#6744))

**Breaking changes**

- `EuiAvatar`s with the default `user` type will now default to
capitalizing all initials in uppercase
([#6739](elastic/eui#6739))

---------

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@spalger spalger

Labels
review v5.0.0-alpha2 v5.0.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@spalger @panda01 @rashidkpc @lukasolson @epixa