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

Move table-responsive class from <table> tag to <div> tag above #1710

Merged
merged 11 commits into from
Dec 26, 2022

Conversation

hannahiss
Copy link
Member

@hannahiss hannahiss commented Dec 15, 2022

Related issues

Linked to issue #1357 and PR #1674

Description

.table-responsive class was misused on some examples: it was positioned on the <table> tag, whereas the documentation says to wrap the table inside a HTML tag (<div>) with .table-responsive.

Motivation & Context

I saw this misuse while correcting some tables row height, on rich content tables examples.

Types of change

  • Bug fix (non-breaking which fixes an issues)

Live previews

Checklist

  • I have read the contributing guidelines
  • My code follows the code style of the project (using npm run lint)
  • My change introduces changes to the documentation
  • My change introduces changes to the migration guide
  • I have updated the documentation accordingly
  • I have added tests to cover my changes
  • All new and existing tests passed

@netlify
Copy link

netlify bot commented Dec 15, 2022

Deploy Preview for boosted ready!

Name Link
🔨 Latest commit c7954fa
🔍 Latest deploy log https://app.netlify.com/sites/boosted/deploys/63a95512aaaf720008d69942
😎 Deploy Preview https://deploy-preview-1710--boosted.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@hannahiss hannahiss changed the title Move table-responsive class from table tag to div tag above Move table-responsive class from <table> tag to <div> tag above Dec 15, 2022
@hannahiss hannahiss marked this pull request as ready for review December 15, 2022 15:57
@julien-deramond julien-deramond added v5 docs Improvements or additions to documentation fix labels Dec 16, 2022
@julien-deramond
Copy link
Member

Was just looking at the overall modifications to check what's the topic of this PR. The rendering seems broken here:
Screenshot 2022-12-16 at 07 55 51
The checkboxes are "glued" to the left hand side of the table.

@hannahiss
Copy link
Member Author

Was just looking at the overall modifications to check what's the topic of this PR. The rendering seems broken here
The checkboxes are "glued" to the left hand side of the table.

Yes, your're right, so... Either we have a problem in the documentation (saying that .table-responsive should be on an element wrapping the <table> tag - which comes from Bootstrap), or we have a problem on the code of rich tables content...?

@julien-deramond
Copy link
Member

There is a specific rule somewhere for the docs that can maybe cause this issue only in the docs:

.bd-content>.table th:first-child,
.bd-content>.table td:first-child,
.bd-content>.table-responsive .table th:first-child,
.bd-content>.table-responsive .table td:first-child {
 padding-left:0;
}

You can try your modification outside of the docs context. Maybe that's just the extra-css for the docs that is the issue.

@julien-deramond
Copy link
Member

If you want to cheat a little bit, you could probably use a .table-responsive-{breakpoint} instead of .table-responsive so that it doesn't trigger the specific docs CSS 😇

@hannahiss
Copy link
Member Author

It seems that this rule:

.bd-content {
  > .table,
  > .table-responsive .table {
    th,
    td {
      &:first-child {
        padding-left: 0;
      }
    }
  }
}

changes almost all tables over documentation, except the ones in Content>Tables section, because they are all wrapped inside <div class="bd-example"> that breaks the rule. By moving .table-responsive to the div above, I reactivated the rule on the rich content tables because they are missing <div class="bd-example"> (which I corrected in PR #1674)

Example from Back To Top > Sass options:

With the css rule:
image

Without the css rule:
image

Isn't it strange that all our tables across documentation do not respect our design specifications ?

Tables from ODS design specs:
image

Why should we keep this strange rule ?

@julien-deramond
Copy link
Member

julien-deramond commented Dec 16, 2022

Since Boosted doc must respect our design guidelines as well. From what I understand, we can probably remove this rule entirely by "Boosted mod"ing it and have a slightly different display compared to Bootstrap.

@julien-deramond
Copy link
Member

There's a rendering regression (size of checkboxes and texts) between the new version:

Screenshot 2022-12-19 at 12 58 50

And the previous one:

Screenshot 2022-12-19 at 12 59 22

@louismaximepiton
Copy link
Member

There's a rendering regression (size of checkboxes and texts) between the new version:

And the previous one:

The issue comes from the fact that the direct .table of .bd-content have their font-size reduced. Shall we changed it ? Otherwise a quick solution here is to wrap those with a .bd-example.

@sonarcloud
Copy link

sonarcloud bot commented Dec 26, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@julien-deramond julien-deramond merged commit e1e520f into main Dec 26, 2022
@julien-deramond julien-deramond deleted the main-his-table-responsive branch December 26, 2022 08:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs Improvements or additions to documentation fix v5
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants