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

[Timelion] Remove kui usage #80287

Merged
merged 12 commits into from
Oct 16, 2020
Merged

[Timelion] Remove kui usage #80287

merged 12 commits into from
Oct 16, 2020

Conversation

stratoula
Copy link
Contributor

@stratoula stratoula commented Oct 13, 2020 •
edited
Loading

Summary

Closes #80052.
It removes kui usage from timelion app. It also removes the usage of FontAwesome.

Checklist

@stratoula
Copy link
Contributor Author

stratoula commented Oct 13, 2020 •
edited
Loading

@cchaos what do you think? I have removed the kui styles and added new ones that are either inline or on the timelion scss files. I also removed fontAwesome usage

@stratoula stratoula requested a review from cchaos October 13, 2020 08:55
cchaos
cchaos reviewed Oct 13, 2020
View reviewed changes
Copy link
Contributor

@cchaos cchaos left a comment

Choose a reason for hiding this comment

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

Wow, that was a lot of work @stratoula ! Thanks for doing that. My only suggestion is to still make sure that all classes are prefixed with .tim to keep them scoped to the Timelion app. Some of the selctors aren't very unique, like form-checkbox, which could end up clashing with a similar selector from someone else.

But other than that, the approach LGTM, but I haven't pulled the PR to test locally. Just wanted to give my initial thoughts first.

@stratoula
Copy link
Contributor Author

Thank you @cchaos! You are totally right. Will fix this and send it for a proper review ❤️

@stratoula
Copy link
Contributor Author

@elasticmachine merge upstream

@stratoula stratoula added Feature:Timelion Timelion app and visualization release_note:skip Skip the PR/issue when compiling release notes v7.11.0 v8.0.0 labels Oct 14, 2020
@stratoula
Copy link
Contributor Author

@elasticmachine merge upstream

@stratoula stratoula added the Team:Visualizations Visualization editors, elastic-charts and infrastructure label Oct 14, 2020
@stratoula stratoula marked this pull request as ready for review October 14, 2020 09:37
@stratoula stratoula requested a review from a team October 14, 2020 09:37
@stratoula stratoula requested a review from a team as a code owner October 14, 2020 09:37
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-app (Team:KibanaApp)

cchaos
cchaos suggested changes Oct 14, 2020
View reviewed changes
Copy link
Contributor

@cchaos cchaos left a comment

Choose a reason for hiding this comment

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

I compared the new version with the one from the Demo site and found just a couple small issues, but everything else seems to be the same.

1. Is it possible to move the menu system to the header area as we do with all the other apps now?

Image 2020-10-14 at 10 31 55 AM

2. The icons in the chart aren't showing up

Screen Shot 2020-10-14 at 10 36 41 AM

3. The auto-complete dropdown isn't connected to the input as well as it was before. Also, the input itself looks to have a darker bg than the rest making it look disabled.

Image 2020-10-14 at 10 35 20 AM

4. The tabs under Help don't change active state (very minor)

Image 2020-10-14 at 10 34 09 AM

src/plugins/timelion/public/directives/_form.scss Outdated Show resolved Hide resolved
src/plugins/timelion/public/directives/_form.scss Outdated Show resolved Hide resolved
src/plugins/timelion/public/directives/timelion_help/timelion_help.html Outdated Show resolved Hide resolved
@stratoula
Copy link
Contributor Author

@cchaos thank you a lot! I will address them all but the first one is not relevant with this PR so I would suggest creating an issue for this and discuss this request there. wdyt?

@stratoula
Copy link
Contributor Author

@elasticmachine merge upstream

@stratoula
Copy link
Contributor Author

stratoula commented Oct 15, 2020 •
edited
Loading

@cchaos so I addressed the 2nd and 3rd but I can't understand the 4th point. Can you elaborate more?
I also created another PR about the first one here

cchaos
cchaos approved these changes Oct 15, 2020
View reviewed changes
Copy link
Contributor

@cchaos cchaos left a comment

Choose a reason for hiding this comment

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

No. 4 actually looks like it is working now. Not sure what changed, but basically the tabs in the Help section weren't updating their "active" state when switching between them. All good now.

LGTM with the exception of replacing a few hex values with EUI vars (for dark mode).

src/plugins/timelion/public/directives/_form.scss Outdated Show resolved Hide resolved
src/plugins/timelion/public/directives/_form.scss Outdated Show resolved Hide resolved
@kertal kertal self-requested a review October 16, 2020 09:13
kertal
kertal approved these changes Oct 16, 2020
View reviewed changes
Copy link
Member

@kertal kertal left a comment

Choose a reason for hiding this comment

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

Code LGTM ,thx for the cleanup and also for removing the font that used to be awesome! Tested locally, and found one minor issue, the "Don't show this again" link/button is displayed in plain text color. Should be an easy CSS fix, so I'm approving and I think it's safe to press the "Don't review this again" button.
New_TimeLion_Sheet_-_Elastic_und_New_TimeLion_Sheet_-_Elastic

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

async chunks size

id before after diff
timelion 382.4KB 393.2KB +10.8KB

page load bundle size

id before after diff
timelion 14.6KB 14.5KB -76.0B

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

@stratoula stratoula merged commit 149b63c into elastic:master Oct 16, 2020
stratoula added a commit to stratoula/kibana that referenced this pull request Oct 16, 2020
@stratoula @kibanamachine
[Timelion] Remove kui usage (elastic#80287)
fa5e15b 
* [Timelion] Remove kui usage

* Fix custom checkbox

* Add tim prefix to the new classes

* Fix functional test

* PR fixes

* Fix type

* PR comments

* Remove the last fontawesome usages

* fix timelion links

Co-authored-by: Kibana Machine <[email protected]>
@stratoula stratoula mentioned this pull request Oct 16, 2020
Merged
stratoula added a commit that referenced this pull request Oct 16, 2020
@stratoula @kibanamachine
[Timelion] Remove kui usage (#80287) (#80819)
1b8c092 
* [Timelion] Remove kui usage

* Fix custom checkbox

* Add tim prefix to the new classes

* Fix functional test

* PR fixes

* Fix type

* PR comments

* Remove the last fontawesome usages

* fix timelion links

Co-authored-by: Kibana Machine <[email protected]>

Co-authored-by: Kibana Machine <[email protected]>
gmmorris added a commit to gmmorris/kibana that referenced this pull request Oct 19, 2020
@gmmorris
Merge branch 'master' into task-manager/health
faefc37 
* master: (51 commits)
  [Discover] Unskip flaky test (elastic#80670)
  Fix security solution template label (elastic#80754)
  [Ingest]: ignore 404, check if there are transforms in results. (elastic#80721)
  Moving loader to logo in header, add a slight 250ms pause (elastic#78879)
  [Security Solution][Cases] Fix bug with case connectors (elastic#80642)
  Update known-plugins.asciidoc (elastic#75388)
  [Lens] Add median operation (elastic#79453)
  Fix navigateToApp logic when navigating to the current app. (elastic#80809)
  [Visualizations] Fix bad color mapping with multiple split series (elastic#80801)
  [ILM] Add esErrorHandler for the new es js client (elastic#80302)
  Fix codeowners (elastic#80826)
  skip flaky suite (elastic#79463)
  [Timelion] Remove kui usage (elastic#80287)
  [Ingest Manager] add skipIfNoDockerRegistry to package_install_complete test (elastic#80779)
  [Alerting UI] Disable "Save" button for Alerts with broken Connectors (elastic#80579)
  Allow the default space to be accessed via `/s/default` (elastic#77109)
  Add script to identify plugin dependencies for TS project references migration (elastic#80463)
  [Search] Client side session service (elastic#76889)
  feat: 🎸 add separator for different context menu groups (elastic#80498)
  Lazy load reporting (elastic#80492)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cchaos cchaos cchaos approved these changes

@kertal kertal kertal approved these changes

Assignees
No one assigned
Labels
Feature:Timelion Timelion app and visualization release_note:skip Skip the PR/issue when compiling release notes Team:Visualizations Visualization editors, elastic-charts and infrastructure v7.11.0 v8.0.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Timelion] Inline used kui styles/elements
5 participants
@stratoula @elasticmachine @kibanamachine @kertal @cchaos