De-angularize discover kbnTableHeader #41259
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
💔 Build Failed |
kertal
force-pushed
the
kertal-pr-doc_table_header
branch
from
a7d7aaa
to
baa5057
Compare
kertal
changed the title
💔 Build Failed |
💔 Build Failed |
- makes more sense than that 1% relict
💚 Build Succeeded |
💔 Build Failed |
💚 Build Succeeded |
💔 Build Failed |
💚 Build Succeeded |
kertal
commented
Jul 22, 2019
| if (!this.fields || !this.fields.byName) return; | ||
| return this.fields.byName[name]; | ||
| } | ||
|
|
There was a problem hiding this comment.
Added the new function to satisfy typescript, also I think it's a better way to access field settings
kertal
commented
Jul 22, 2019
src/legacy/core_plugins/kibana/public/discover/doc_table/components/_table_header.scss
Show resolved
Hide resolved
kertal
commented
Jul 22, 2019
|
|
||
| return ( | ||
| <tr data-test-subj="docTableHeader" className="kbnDocTableHeader"> | ||
| <th style={{ width: '24px' }}></th> |
There was a problem hiding this comment.
In the original angular template this was 1%, of course this should be removed generally when the whole table is reactified + EUItified
There was a problem hiding this comment.
Should we add a TODO or a ticket here so future people know what's up with this (even though there's a good chance the future people are you, seems like good practice)
There was a problem hiding this comment.
This won't survive EUIfication, which will follow. It's just to keep the old way for a while, a bit more improved. There's no way this is kept, so I think in this case it's ok not to add a TODO here.
💚 Build Succeeded |
💔 Build Failed |
|
jenkins test this |
💚 Build Succeeded |
myasonik
approved these changes
Jul 25, 2019
|
|
||
| return ( | ||
| <tr data-test-subj="docTableHeader" className="kbnDocTableHeader"> | ||
| <th style={{ width: '24px' }}></th> |
There was a problem hiding this comment.
Should we add a TODO or a ticket here so future people know what's up with this (even though there's a good chance the future people are you, seems like good practice)
kertal
added a commit
to kertal/kibana
that referenced
this pull request
Jul 25, 2019
* Add TableHeader + TableHeaderColumn react components * Migration of kbnTableHeader to use react component * Disable mocha tests * Add jest tests
💔 Build Failed |
3 tasks
kertal
added a commit
that referenced
this pull request
Jul 25, 2019
1 task
Bargs
added a commit
to Bargs/kibana
that referenced
this pull request
Jul 26, 2019
…elastic#42026) Prior to elastic#41259 we intentionally did not show the sort, move, and remove column buttons in the doc table in the Context app. Context doesn't pass in handlers for these actions and in the old angular directive we conditionally showed the buttons depending on whether those handlers were passed in. So in this PR I've simply mimicked that behavior in the new React component.
Bargs
added a commit
that referenced
this pull request
Jul 26, 2019
…#42026) Prior to #41259 we intentionally did not show the sort, move, and remove column buttons in the doc table in the Context app. Context doesn't pass in handlers for these actions and in the old angular directive we conditionally showed the buttons depending on whether those handlers were passed in. So in this PR I've simply mimicked that behavior in the new React component.
7 tasks
Bargs
added a commit
that referenced
this pull request
Jul 26, 2019
…#42026) (#42082) Prior to #41259 we intentionally did not show the sort, move, and remove column buttons in the doc table in the Context app. Context doesn't pass in handlers for these actions and in the old angular directive we conditionally showed the buttons depending on whether those handlers were passed in. So in this PR I've simply mimicked that behavior in the new React component.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
Rewrite
doc_tabletable header (kbnTableHeaderdirective) in React, focused on removing React. EUIfication when all parts of the table are written in React.Part of #38646
Use
strikethroughsto remove checklist items you don't feel are applicable to this PR.- [ ] 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] (https://github.com/elastic/kibana/blob/master/CONTRIBUTING.md#cross-browser-compatibility) were updated or added to match the most common scenarios- [ ] This was checked for keyboard-only and screenreader accessibilityFor maintainers
- [ ] This was checked for breaking API changes and was labeled appropriately- [ ] This includes a feature addition or change that requires a release note and was labeled appropriately