-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
Remove bootstrap dependency. #7567
Remove bootstrap dependency. #7567
Conversation
I spoke with @cjcenizal about bringing |
@cjcenizal maybe it makes sense to merge this PR and then start removing the other bootstrap components? I'm thinking it's going to take a while to review all of those sub-PRs, and then this one meta-PR is going to have to be re-reviewed with all of them merged. I think it's best to merge things into master incrementally if possible. |
@Bargs I'm game. @tylersmalley What do you think? |
It's probably a good idea. It would be much easier to review the refactoring which was done as individual PRs. This PR as is should not have any impact on the UI as it's just moving the dependency into the repo. |
@Bargs @tylersmalley OK I updated the description. Can you give me some LGTMs? Then I'll merge. |
// | ||
//## | ||
|
||
@carousel-text-shadow: 0 1px 2px rgba(0,0,0,.6); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where did these go?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After removing the carousel classes, there are no more references to these variables so I felt it was safe to remove them.
I see some references to glyphicons in Same question applies to carousel now that I search for it. |
Not really a big deal, but maybe these comments should be updated: https://github.com/elastic/kibana/blob/master/src/ui/public/styles/dark-theme.less#L8 |
@Bargs Great catch re glyphicons. Looks like we may be using two components (daypicker and timepicker) which rely on a few specific glyphicons, so I'll update the PR to retain those. We're not using the other two components (carousel and rating) which use glyphicons. |
@Bargs I'll put the carousel classes back and remove them along with the Angular stuff in a later PR. |
Sounds like a plan. I think it'll be nice for historical reasons to have a PR that only moves the files around, without changing anything anyways. That way we have a good base to look at in the commit history. |
983cb05
to
0b2a8cc
Compare
@Bargs @tylersmalley Could you please re-review? I put back carousel and glyphicons. |
LGTM |
…into src/ui/public/styles/bootstrap and /fonts.
ee44a34
to
00efd32
Compare
LGTM |
…rap-dependency Remove bootstrap dependency. Former-commit-id: 2cf2218
Relates to #7364.
Changes
import
statements.Next steps
File size
It's worth noting that the original filesize of our compiled CSS was 949kb. Already, by removing the glyphicons and carousel LESS, this filesize has dropped to 890kb.