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

Remove unused Bootstrap label and well classes #7686

Conversation

cjcenizal
Copy link
Contributor

@cjcenizal cjcenizal commented Jul 8, 2016

Doesn't look like these are used by either Kibana or X-Pack.

@Bargs Bargs self-assigned this Jul 13, 2016
@Bargs Bargs added the review label Jul 13, 2016
@Bargs
Copy link
Contributor

Bargs commented Jul 18, 2016

It seems kind of strange to be removing most of the label/well related classes, but not all of them. What happens if someone needs a label-success type class between the time this PR gets merged, and the time we add our own label component? Someone has to invent their own label-success, and then we potentially have two different label components to replace down the road?

@cjcenizal
Copy link
Contributor Author

@Bargs Well, think of this as code which we've written ourselves. Someone a couple years ago wrote some classes that represent a previous UI direction which has now drastically changed. The classes in question are no longer in use, and at this point we don't anticipate them being used in the future (since they're not a part of the new design direction).

In this case, I think we should remove the classes because they're not supposed to be used in the UI. They don't adhere to the new design, so using them in the UI would be a mistake and we'd have to spend time fixing them. They're worse than zombie code... they're zombies waiting to wake up and cause damage.

If it turns out that we do need some sort of label to represent a success state, then I would expect @alt74 to provide guidance on how that looks and the role it plays in the UX, and then we can add a label-success class that fulfills his direction. I would expect it to be as straight-forward as copy-pasting an existing label modifier and passing in a different background color.

@cjcenizal
Copy link
Contributor Author

jenkins, test this

@cjcenizal
Copy link
Contributor Author

cjcenizal commented Jul 18, 2016

jenkins, test this and don't you dare fail again!

@Bargs
Copy link
Contributor

Bargs commented Jul 18, 2016

Shouldn't we pull the bandaid off completely and delete the label-warning class as well then? It seems like it's only used in one place. Perhaps I don't really understand the goal here.

@cjcenizal
Copy link
Contributor Author

@Bargs The goal is to remove all unused selectors and classes. So if label-warning is used in one place then I think we should leave it alone, unless we can swap it out for another pre-existing class, or you can think of some other way to get rid of it elegantly?

@cjcenizal
Copy link
Contributor Author

jenkins, test this

2 similar comments
@cjcenizal
Copy link
Contributor Author

jenkins, test this

@cjcenizal
Copy link
Contributor Author

jenkins, test this

@Bargs
Copy link
Contributor

Bargs commented Jul 26, 2016

@cjcenizal and spoke on zoom with @spalger, who had a crazy idea for isolating these old styles. If it pans out, we might not get much benefit from deleting these old styles piecemeal. So this PR is on hold while @spalger validates his idea.

@Bargs Bargs removed their assignment Jul 26, 2016
@cjcenizal
Copy link
Contributor Author

Closing this because its value has been superseded by #7899

@cjcenizal cjcenizal closed this Aug 16, 2016
@cjcenizal cjcenizal deleted the refactor/remove-unused-bootstrap-classes branch August 16, 2016 22:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants