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

Timelion less to sass #23339

Merged
merged 35 commits into from
Sep 21, 2018
Merged

Timelion less to sass #23339

merged 35 commits into from
Sep 21, 2018

Conversation

cchaos
Copy link
Contributor

@cchaos cchaos commented Sep 19, 2018

This PR removes the LESS files for the Timeline plugin and replaces them with Sass.

Process taken

  1. The src/core_plugins/timelion/index.js file was updated to build src/core_plugins/timelion/public/index.scss into a css file.
    • That index file imports styling_constants.scss from ui/public/styles in Kibana. This a placeholder file that includes all theming and invisible sass globals from EUI.
    • Any sass files under that line can now use the functions, mixins and variables from EUI. Although the theme is hardcoded, it will be relatively easy to switch themes based on this import later.
  2. All Less files in the timelion/public/... directory were replaced with sass counterparts.
    • The sass files now live with separated _index.scss and _component_names.scss files next to the components or views they live next to.
    • The Less files were then deleted entirely.
    • The new sass files use EUI variables whenever possible. The most important being color and sizing variables.
    • The selectors were all changed to match EUI's BEM formatting. This means the html/js templating was touched as well.
    • Additionally, a three-letter prefix tim was added to all selectors to namespace them and avoid conflicts.

Enhancements

The help/tutorial section at the top had no text styles whatsover. I added the .euiText class to the wrapping div just to apply some basic semantic styles:

Before

screen shot 2018-09-19 at 17 36 32 pm

After

screen shot 2018-09-19 at 17 35 48 pm


@elastic/kibana-app or @elastic/kibana-visualizations teams, please assign a reviewer


cchaos and others added 30 commits August 29, 2018 10:54
- Moves styles closer to their actual components in `public/vis/…` (agg stuff did not get naming updates)
- Also fixes responsive layout to actually show the visualization
- Updated to `<icon/>` EUI usage where possible
# Conflicts:
#	src/core_plugins/kibana/public/visualize/editor/styles/_editor.less
- And fixed up Markdown options (the only one) that relied on it
- Removed unnecessary flex parents
- Also updated ems-hotlink
- The embeddable `/vis` now imports the compiled `index.css` that lives one folder up
…o-sass

# Conflicts:
#	src/core_plugins/input_control_vis/public/register_vis.js
#	src/core_plugins/markdown_vis/public/markdown_vis.js
#	src/core_plugins/markdown_vis/public/markdown_vis_params.html
#	src/ui/public/styles/variables/for-theme.less
#	src/ui/public/vis/editors/default/_agg_group.scss
#	src/ui/public/vis/editors/default/_index.scss
#	src/ui/public/vis/editors/default/agg_group.html
#	x-pack/plugins/ml/public/jobs/new_job/wizard/steps/index_or_search/index_or_search.html
@cchaos
Copy link
Contributor Author

cchaos commented Sep 19, 2018

Ugh, it looks like the error is happening because the bundler hasn't built the timelion/public/_index.scss into as CSS file yet, so ../index.css doesn't exist. So my "clever" way of importing the TL embedded vis styles won't work.

I've updated the PR to actually import the scss files it needs for embedded visualizations in src/core_plugins/kibana/public/index.scss

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

* 1. Anchor suggestions beneath input.
* 2. Allow for option of positioning suggestions absolutely.
*/
.timExpressionInput__container {
Copy link
Contributor

Choose a reason for hiding this comment

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

🎨 unnecessary leading space

</div>
</div>
<div ng-show="page === 2">
<div ng-show="!es.valid">
<div class="doc-container-content">
<div >
Copy link
Contributor

Choose a reason for hiding this comment

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

🎨 remove that unnecessary space

Copy link
Contributor

@timroes timroes left a comment

Choose a reason for hiding this comment

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

Just a couple of minor style suggestions, but looks good to me otherwise. Tested in Chrome Linux, can't see any obvious styling issues.

</div>
</div>
<div ng-show="es.valid">
<div class="doc-container-content">
<div >
Copy link
Contributor

Choose a reason for hiding this comment

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

🎨 space

</div>
</div>
</div>
<div ng-show="page === 3">
<div class="doc-container-content">
<div >
Copy link
Contributor

Choose a reason for hiding this comment

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

🎨 space

</div>
</div>

<div ng-show="page === 4">
<div class="doc-container-content">
<div >
Copy link
Contributor

Choose a reason for hiding this comment

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

🎨 space

</div>
</div>
<div ng-show="page === 5">
<div class="doc-container-content">
<div >
Copy link
Contributor

Choose a reason for hiding this comment

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

🎨 space

@elasticmachine
Copy link
Contributor

💔 Build Failed

@cchaos
Copy link
Contributor Author

cchaos commented Sep 20, 2018

✖ fail: "apis saved_objects Kibana index migration Coordinates migrations across the Kibana cluster"

Seems flakey, Jenkins, test this

@elasticmachine
Copy link
Contributor

💔 Build Failed

@cchaos
Copy link
Contributor Author

cchaos commented Sep 20, 2018

OI... unknown error: Chrome failed to start: exited abnormally

Jenkins, test this

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@cchaos
Copy link
Contributor Author

cchaos commented Sep 21, 2018

@snide Do you want to take a quick peruse?

Copy link
Contributor

@snide snide left a comment

Choose a reason for hiding this comment

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

Looks good.

@cchaos
Copy link
Contributor Author

cchaos commented Sep 21, 2018

Thanks @timroes and @snide !

@cchaos cchaos merged commit 7991a38 into elastic:master Sep 21, 2018
@cchaos cchaos deleted the timelion-less-to-sass branch September 21, 2018 04:06
chrisronline pushed a commit that referenced this pull request Sep 21, 2018
cchaos added a commit to cchaos/kibana that referenced this pull request Sep 21, 2018
cchaos added a commit that referenced this pull request Sep 22, 2018
chrisronline added a commit that referenced this pull request Sep 24, 2018
* Merge in boilerplate branch

* Manually copy over the specific metrics and UIs

* Add api integration tests

* Fix tests

* Remove unused metrics

* Update snapshot

* Fix tests

* Remove types agg

* Use ApmClusterMetric

* provide description for apm-server monitoring metrics (#23331)

* Vis LESS to SASS (cont.) (#23199)

* Tweak migrations integraiton tests to have a stable sort (#23265)

* Fix: plugin api route with security enabled (#23334)

Closes #23266

This is more of a quick fix than the final solution. The issue was that Canvas tries to check the plugins API without checking to see if the user it logged in. As a result, instead of the plugins response, it gets the HTML from the login page and that causes an error to be thrown when attempting to parse the results.

For now, this PR just disables the auth requirement on the Canvas plugin API endpoint.

* [migrations/tests] sort results before assertion (#23347)

There have been several failures in this test, seemingly caused by a lack of sorting in the results. It makes sense that since both migrations are run simultaneously that sometimes one would succeed and sometimes another would, so I've just sorted the results before checking.

![image](https://user-images.githubusercontent.com/1329312/45791153-44e9cc80-bc3d-11e8-88c4-760d4c7b35bd.png)

cc: @chrisdavies

* [ML] Moves custom URL editor Add button and form to top of flyout (#23326)

* [ML] Moves custom URL editor Add button and form to top of flyout

* [ML] Edits to custom URL editor class name

* Graph LESS to SASS (#23348)

* Developer documentation for integrating with the telemetry service (#23295)

* Developer documentation for integrating with the telemetry service

* open with a bang

* more faqs

* thing about tracking ui interactions

* talk to the plat team

* create and register

* Fix a bug where ES sends a string and migrations expect a boolean (#23313)

* chore: use cheerio in i18n.html.getDirectiveMessages (#23342)

this was only using jsdom to parse html, but cheerio allows parsing html without requiring a dom. cheerio was also already in the dependency list.

* [core/utils] add shareWeakReplay() operator (#23333)

* Chore: fix canvas test runner (#23336)

Blocked by #23342

This fixes the local test runner in Canvas. It should not affect anything else, including the CI test runner.

- Bumps JSDOM to ^12.0.0
  - I matched Kibana's version on migration, but nothing else in X-Pack uses JSDOM, so we can use the newer version (which has a very different API)
  - I had to match it because of a script that enforces version matching, but #23342 removed jsdom from Kibana, so we no longer have a version to match
- Restores the local `.babelrc` file
  - I thought it was only used for building plugins; I was wrong 😢

* Convert Discover open top nav to EUI flyout (#22971)

* move find logic to SavedObjectFinder component since savedObjectClient is no longer coupled to angular

* implement flyout open saved searches

* remove old open stuff

* add jest test for OpenSearchPanel and simplify panel title

* fix functional tests

* fix _lab_mode functional test

* Migrate save top nav in Discover and Visualize to EUI (#23190)

* extract reusable save component from DashboardSaveModal

* update discover search to use SavedObjectSaveModal

* create generic show_save_model that works for both discover and dashboard

* fix last bits of discover save

* remove old save functionallity

* migrate visualize save to EUI

* fix functional tests

* disable save button if title is empty

* mark title input as invalid when title is not provided

* fix funtional tests

* Moves styleSheetPath to uiExports (#23007)

This was previously defined in uiExports.app, which limited plugins which are not an app of providing a stylesheet. This allows any plugin to define a stylesheet which will be available on page load.

* Timelion less to sass (#23339)

* Consistent casing

* Fix snapshot

* Update tests
chrisronline added a commit to chrisronline/kibana that referenced this pull request Sep 24, 2018
* Merge in boilerplate branch

* Manually copy over the specific metrics and UIs

* Add api integration tests

* Fix tests

* Remove unused metrics

* Update snapshot

* Fix tests

* Remove types agg

* Use ApmClusterMetric

* provide description for apm-server monitoring metrics (elastic#23331)

* Vis LESS to SASS (cont.) (elastic#23199)

* Tweak migrations integraiton tests to have a stable sort (elastic#23265)

* Fix: plugin api route with security enabled (elastic#23334)

Closes elastic#23266

This is more of a quick fix than the final solution. The issue was that Canvas tries to check the plugins API without checking to see if the user it logged in. As a result, instead of the plugins response, it gets the HTML from the login page and that causes an error to be thrown when attempting to parse the results.

For now, this PR just disables the auth requirement on the Canvas plugin API endpoint.

* [migrations/tests] sort results before assertion (elastic#23347)

There have been several failures in this test, seemingly caused by a lack of sorting in the results. It makes sense that since both migrations are run simultaneously that sometimes one would succeed and sometimes another would, so I've just sorted the results before checking.

![image](https://user-images.githubusercontent.com/1329312/45791153-44e9cc80-bc3d-11e8-88c4-760d4c7b35bd.png)

cc: @chrisdavies

* [ML] Moves custom URL editor Add button and form to top of flyout (elastic#23326)

* [ML] Moves custom URL editor Add button and form to top of flyout

* [ML] Edits to custom URL editor class name

* Graph LESS to SASS (elastic#23348)

* Developer documentation for integrating with the telemetry service (elastic#23295)

* Developer documentation for integrating with the telemetry service

* open with a bang

* more faqs

* thing about tracking ui interactions

* talk to the plat team

* create and register

* Fix a bug where ES sends a string and migrations expect a boolean (elastic#23313)

* chore: use cheerio in i18n.html.getDirectiveMessages (elastic#23342)

this was only using jsdom to parse html, but cheerio allows parsing html without requiring a dom. cheerio was also already in the dependency list.

* [core/utils] add shareWeakReplay() operator (elastic#23333)

* Chore: fix canvas test runner (elastic#23336)

Blocked by elastic#23342

This fixes the local test runner in Canvas. It should not affect anything else, including the CI test runner.

- Bumps JSDOM to ^12.0.0
  - I matched Kibana's version on migration, but nothing else in X-Pack uses JSDOM, so we can use the newer version (which has a very different API)
  - I had to match it because of a script that enforces version matching, but elastic#23342 removed jsdom from Kibana, so we no longer have a version to match
- Restores the local `.babelrc` file
  - I thought it was only used for building plugins; I was wrong 😢

* Convert Discover open top nav to EUI flyout (elastic#22971)

* move find logic to SavedObjectFinder component since savedObjectClient is no longer coupled to angular

* implement flyout open saved searches

* remove old open stuff

* add jest test for OpenSearchPanel and simplify panel title

* fix functional tests

* fix _lab_mode functional test

* Migrate save top nav in Discover and Visualize to EUI (elastic#23190)

* extract reusable save component from DashboardSaveModal

* update discover search to use SavedObjectSaveModal

* create generic show_save_model that works for both discover and dashboard

* fix last bits of discover save

* remove old save functionallity

* migrate visualize save to EUI

* fix functional tests

* disable save button if title is empty

* mark title input as invalid when title is not provided

* fix funtional tests

* Moves styleSheetPath to uiExports (elastic#23007)

This was previously defined in uiExports.app, which limited plugins which are not an app of providing a stylesheet. This allows any plugin to define a stylesheet which will be available on page load.

* Timelion less to sass (elastic#23339)

* Consistent casing

* Fix snapshot

* Update tests
chrisronline added a commit that referenced this pull request Sep 24, 2018
* Merge in boilerplate branch

* Manually copy over the specific metrics and UIs

* Add api integration tests

* Fix tests

* Remove unused metrics

* Update snapshot

* Fix tests

* Remove types agg

* Use ApmClusterMetric

* provide description for apm-server monitoring metrics (#23331)

* Vis LESS to SASS (cont.) (#23199)

* Tweak migrations integraiton tests to have a stable sort (#23265)

* Fix: plugin api route with security enabled (#23334)

Closes #23266

This is more of a quick fix than the final solution. The issue was that Canvas tries to check the plugins API without checking to see if the user it logged in. As a result, instead of the plugins response, it gets the HTML from the login page and that causes an error to be thrown when attempting to parse the results.

For now, this PR just disables the auth requirement on the Canvas plugin API endpoint.

* [migrations/tests] sort results before assertion (#23347)

There have been several failures in this test, seemingly caused by a lack of sorting in the results. It makes sense that since both migrations are run simultaneously that sometimes one would succeed and sometimes another would, so I've just sorted the results before checking.

![image](https://user-images.githubusercontent.com/1329312/45791153-44e9cc80-bc3d-11e8-88c4-760d4c7b35bd.png)

cc: @chrisdavies

* [ML] Moves custom URL editor Add button and form to top of flyout (#23326)

* [ML] Moves custom URL editor Add button and form to top of flyout

* [ML] Edits to custom URL editor class name

* Graph LESS to SASS (#23348)

* Developer documentation for integrating with the telemetry service (#23295)

* Developer documentation for integrating with the telemetry service

* open with a bang

* more faqs

* thing about tracking ui interactions

* talk to the plat team

* create and register

* Fix a bug where ES sends a string and migrations expect a boolean (#23313)

* chore: use cheerio in i18n.html.getDirectiveMessages (#23342)

this was only using jsdom to parse html, but cheerio allows parsing html without requiring a dom. cheerio was also already in the dependency list.

* [core/utils] add shareWeakReplay() operator (#23333)

* Chore: fix canvas test runner (#23336)

Blocked by #23342

This fixes the local test runner in Canvas. It should not affect anything else, including the CI test runner.

- Bumps JSDOM to ^12.0.0
  - I matched Kibana's version on migration, but nothing else in X-Pack uses JSDOM, so we can use the newer version (which has a very different API)
  - I had to match it because of a script that enforces version matching, but #23342 removed jsdom from Kibana, so we no longer have a version to match
- Restores the local `.babelrc` file
  - I thought it was only used for building plugins; I was wrong 😢

* Convert Discover open top nav to EUI flyout (#22971)

* move find logic to SavedObjectFinder component since savedObjectClient is no longer coupled to angular

* implement flyout open saved searches

* remove old open stuff

* add jest test for OpenSearchPanel and simplify panel title

* fix functional tests

* fix _lab_mode functional test

* Migrate save top nav in Discover and Visualize to EUI (#23190)

* extract reusable save component from DashboardSaveModal

* update discover search to use SavedObjectSaveModal

* create generic show_save_model that works for both discover and dashboard

* fix last bits of discover save

* remove old save functionallity

* migrate visualize save to EUI

* fix functional tests

* disable save button if title is empty

* mark title input as invalid when title is not provided

* fix funtional tests

* Moves styleSheetPath to uiExports (#23007)

This was previously defined in uiExports.app, which limited plugins which are not an app of providing a stylesheet. This allows any plugin to define a stylesheet which will be available on page load.

* Timelion less to sass (#23339)

* Consistent casing

* Fix snapshot

* Update tests
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
chore Feature:Timelion Timelion app and visualization Team:Platform-Design Team Label for Kibana Design Team. Support the Analyze group of plugins. Team:Visualizations Visualization editors, elastic-charts and infrastructure v6.5.0 v7.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants