Skip to content

Rename QueryBar to QueryBarTopRow#45250

Merged
lizozom merged 6 commits intoelastic:masterfrom
lizozom:newplatform/query/rename-query-bar
Sep 13, 2019
Merged

Rename QueryBar to QueryBarTopRow#45250
lizozom merged 6 commits intoelastic:masterfrom
lizozom:newplatform/query/rename-query-bar

Conversation

@lizozom
Copy link
Contributor

@lizozom lizozom commented Sep 10, 2019

Summary

Rename QueryBar to QueryBarTopRow and make it an implementation detail of the query service.

Dev Docs

  • Rename QueryBar to QueryBarTopRow
  • Stop exporting QueryBar on the public contract.

Checklist

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

For maintainers

@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-app-arch

@lizozom lizozom added the release_note:plugin_api_changes Contains a Plugin API changes section for the breaking plugin API changes section. label Sep 10, 2019
@lizozom lizozom requested review from a team, Bargs and mattkime and removed request for Bargs September 10, 2019 11:03
@elasticmachine
Copy link
Contributor

💔 Build Failed

Bargs
Bargs previously requested changes Sep 10, 2019
Copy link
Contributor

@Bargs Bargs left a comment

Choose a reason for hiding this comment

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

The SearchBar component is intended to work anywhere, not just the global top row necessarily, so I wouldn't recommend referencing TopRow in the QueryBar component's name.

@lizozom
Copy link
Contributor Author

lizozom commented Sep 11, 2019

@Bargs QueryBar is a sub-component of SearchBar, which is never used externally.
Applications would either consume SearchBar as a whole or QueryBarInput if they want just the input component.

You can see that I didn't only rename the component, but I also stopped exporting it from the plugin's API, so it can't be used by anyone outside the plugin.

Does this make it clearer?

Copy link
Contributor

@ppisljar ppisljar left a comment

Choose a reason for hiding this comment

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

LGTM

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@Bargs
Copy link
Contributor

Bargs commented Sep 11, 2019

@lizozom Why include TopRow in the name though? TopRow makes me think it's referring to the fact that it is usually in the TopNav. Does TopRow just refer to the fact that it is in the first row of the SearchBar? Why include the position in the name at all?

@ppisljar
Copy link
Contributor

we are renaming it as we want to rename searchbar to querybar later.
so if i understand correctly, querybartoprow is not part of our contract, its internal implenentation of querybar (currently known as searchbar) ... i am guessing that its the top part of the query bar (the current search bar).

as its not something we would expose on our contracts i really think we shoulnt loose any time on naming discussions here

@lizozom
Copy link
Contributor Author

lizozom commented Sep 12, 2019

retest

1 similar comment
@lizozom
Copy link
Contributor Author

lizozom commented Sep 12, 2019

retest

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@lizozom
Copy link
Contributor Author

lizozom commented Sep 12, 2019

@Bargs as @ppisljar said, we are making this component internal, so I just renamed it to something easy and specific, to free up the name QueryBar as we're going to need it later, to rename SearchBar.

I would appreciate your approval.

@Bargs
Copy link
Contributor

Bargs commented Sep 12, 2019

as its not something we would expose on our contracts i really think we shoulnt loose any time on naming discussions here

This wasn't a silly naming discussion, I kept asking because the name made no sense to me whatsoever and no one was answering my relatively simple question about what the name actually meant. I want to ensure there is not a misunderstanding about where and how the SearchBar component is meant to be used.

i am guessing that its the top part of the query bar (the current search bar).

If this guess is correct (please confirm @lizozom) then that's fine with me.

@lizozom
Copy link
Contributor Author

lizozom commented Sep 13, 2019

@Bargs QueryBar is the top row of the search bar - a wrapper around LanguageSwitcher, 'QueryInputBar', EuiSuperTimepicker and EuiRefreshButton.

As you (correctly) moved the logic to SearchBar, were removing QueryBar from the contract.

@lizozom lizozom dismissed Bargs’s stale review September 13, 2019 13:11

Agreed in thread

@lizozom lizozom merged commit e4225a7 into elastic:master Sep 13, 2019
lizozom pushed a commit to lizozom/kibana that referenced this pull request Sep 13, 2019
* rename QueryBar to QueryBarTopRow

* Fixed test typo

* Fixed search bar mock
wylieconlon pushed a commit that referenced this pull request Sep 13, 2019
* rename QueryBar to QueryBarTopRow

* Fixed test typo

* Fixed search bar mock
@lizozom lizozom deleted the newplatform/query/rename-query-bar branch November 14, 2019 13:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Feature:NP Migration release_note:plugin_api_changes Contains a Plugin API changes section for the breaking plugin API changes section. review v7.5.0 v8.0.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants