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

Replace role=main by main elements #12671

Merged
merged 4 commits into from
Jul 12, 2017
Merged

Conversation

timroes
Copy link
Contributor

@timroes timroes commented Jul 5, 2017

Replace all role="main" by main elements as requested in #12632.

Since we support IE11, the display: block for main is still needed.

Fixes #12632

@timroes timroes added Project:Accessibility Team:Platform-Design Team Label for Kibana Design Team. Support the Analyze group of plugins. labels Jul 5, 2017
@timroes timroes requested a review from cjcenizal July 5, 2017 17:08
@cjcenizal cjcenizal requested a review from aphelionz July 6, 2017 17:15
Copy link
Contributor

@cjcenizal cjcenizal left a comment

Choose a reason for hiding this comment

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

Nice! One small request.

@@ -54,6 +54,10 @@ code {
word-wrap: break-word;
}

main {
display: block;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we move this to https://github.com/elastic/kibana/blob/master/ui_framework/components/_common_styles.scss and add a comment?

/**
 * 1. Required for IE11.
 */
main {
  display: block; /* 1 */
}

@timroes timroes dismissed cjcenizal’s stale review July 6, 2017 17:47

Moved the CSS.

@timroes
Copy link
Contributor Author

timroes commented Jul 7, 2017

Jenkins, test this

Copy link
Contributor

@cjcenizal cjcenizal left a comment

Choose a reason for hiding this comment

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

🍀 Excellent!

Copy link
Contributor

@cjcenizal cjcenizal left a comment

Choose a reason for hiding this comment

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

Oops I approved too soon! Found one small thing.

*/
main {
display: block; /* 1 */
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Oh actually, now that this is part of the UI Framework, we need to recompile and commit the CSS. Just run npm run uiFramework:start to kick off compilation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Are you sure, this is still needed? Running the task (or also uiFramework:build doesn't change any files managed by git. Do I need to copy the compiled files somewhere?

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah I think I should probably rename that task. npm run uiFramework:start will recompile the compiled CSS, whereas uiFramework:build will only build the documentation site.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@cjcenizal I ran both of the tasks, none of them touches git managed files for me?

Copy link
Contributor

Choose a reason for hiding this comment

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

I created timroes#1 for now. We can walk through the task when you're done with X-School to figure out what's going on. 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Merged, and we figure out next week what's wrong with my buildsystem :D

Copy link
Contributor

@aphelionz aphelionz left a comment

Choose a reason for hiding this comment

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

Approving after @cjcenizal's comments are addressed

Copy link
Contributor

@cjcenizal cjcenizal left a comment

Choose a reason for hiding this comment

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

🍰

@timroes timroes merged commit 3fe58e9 into elastic:master Jul 12, 2017
@timroes timroes deleted the replace-main-role branch July 12, 2017 15:30
@timroes timroes restored the replace-main-role branch July 12, 2017 15:33
@timroes timroes deleted the replace-main-role branch July 12, 2017 15:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Project:Accessibility Team:Platform-Design Team Label for Kibana Design Team. Support the Analyze group of plugins.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants