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 some Bootstrap components/classes with Ant analogues #4588

Closed
wants to merge 8 commits into from

Conversation

kravets-levko
Copy link
Collaborator

@kravets-levko kravets-levko commented Jan 24, 2020

What type of PR is this? (check all applicable)

  • Refactor
  • Other

Description

What's changed:

  • replaced Bootstrap's .list-group with Antd <List> (QuerySelector component and Home page);
  • removed Bootstrap form classes (.form-group, .form-control);
  • replaced Bootstrap's .badge with Antd <Badge> (System Status page);
  • replaced markup based on .btn-favourite + .fa-star with <FavoritesControl> component (and added a readonly mode to it) - for consistency;
  • replace Bootstrap's buttons (.btn) - with <Button> in Parameters component (see screenshot) and Query List page's empty state block (looks almost the same). There was also usage in VisualizationWidget component but it didn't really need that .btn styles (most were overridden) - so I just simplified a markup and removed classes at all.

What's left (will be fixed in this PR only after discussion if we decide so):

  • a lot of helper classes: m-*, p-*, bg-*, c-*, f-*, text-*, w-*, h-*, display (d-*) and flex markup. They are actually not a part of Bootstrap (maybe just some of text-* ones) - so probably they should be collected into a single file (or few files) and kept apart from our custom Bootstrap theme;
  • .table-responsive, .table-bordered - there are few usages, need to check what could be used as a replacement;
  • .hidden-*, .visible-* - same as for table classes above;
  • Bootstrap grid: .container, .row, .col-* - not that much usages, may be replaced as a part of this PR (after discussing);
  • .label-* classes: part of Bootstrap, but currently used only as tags labels. May be replaced as a part of this PR (after discussing).

Also, Bootstrap defines some styles for plain tags (e.g. headers, typography, etc.) - need to check if Ant has something similar and how it fits.

Mobile & Desktop Screenshots/Recordings (if there are UI changes)

Parameters components with updated button:
image

@@ -250,6 +250,31 @@
}
}

.@{list-prefix-cls}-bordered.@{list-prefix-cls}-sm {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The actual change, other is from Prettier


// set some defaults for badge
.@{badge-prefix-cls} {
&-count {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The actual change, other is from Prettier

Copy link
Member

@gabrieldutra gabrieldutra left a comment

Choose a reason for hiding this comment

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

Another weird thing I noticed is that the flakyness in the Percy snapshot for TimeAgo is reintroduced, it is supposed to be fixed after #4365. I haven't found its cause yet, but saying in case something pops in your mind.


.parameter-block[data-editable] & {
min-width: calc(100% - 27px); // make room for settings button
max-width: 195px - 27px;
Copy link
Member

@gabrieldutra gabrieldutra Feb 1, 2020

Choose a reason for hiding this comment

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

It may be trickier to reach the same Label behavior for parameters:

Before
image
After
image

The idea is to keep it with a flexible width, if the label itself has more text and the width is not in it's limit, the whole parameter can take up more space 🙂. You couldn't get it to work with the previous css or you just wanted to move it to flex?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I wanted to make it flex (and get rid of calc). Thank you for pointing, I'll check this once more 👍

@guidopetri
Copy link
Contributor

@kravets-levko , thanks for the PR! We've updated a lot of things now that we're Community-driven so - if you're still interested in getting this merged - would you mind rebasing off master to re-run the CI, as well as updating merge conflicts?

We're trying to clean up our PR todo list, so if you're not interested, that's fine - we'll close the PR in about a week if we don't hear back. If you're interested in reopening the PR afterwards, we would also very much welcome that.

@guidopetri guidopetri closed this Jul 21, 2023
@guidopetri guidopetri deleted the replace-bootstrap-components branch July 22, 2023 03:17
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.

3 participants