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

JS docs formatting tweaks #31337

Merged
merged 2 commits into from Mar 14, 2022
Merged

JS docs formatting tweaks #31337

merged 2 commits into from Mar 14, 2022

Conversation

mdo
Copy link
Member

@mdo mdo commented Jul 20, 2020

This PR revisits the options, methods, and events sections for each component's JS section. Builds off @rohit2sharma95's suggested changes for the dispose method and takes a more holistic approach to the sections.

  • All sections are now Markdown-powered tables
  • All tables are using the bs-table short code to add our .table-* classes as needed
  • Does a little bit of wordsmithing, but could use more editing overall

I'm waiting on @Johann-S or someone else to tell me what happens with dispose before calling content final here, but wanted to open the PR for when we're ready.

TODO:

  • Convert the remaining HTML tables to Markdown
  • Verify that the CSS still works and is adapted
  • When we are done converting all tables, see if the markdown shortcode is still used

Preview: https://deploy-preview-31337--twbs-bootstrap.netlify.app/

Fixes #31189

@XhmikosR XhmikosR requested a review from Johann-S July 31, 2020 14:03
@XhmikosR
Copy link
Member

XhmikosR commented Jul 31, 2020

We should make the table responsive BTW.

EDIT: And maybe use normal font-size? Not sure what the rationale behind this choice was.

@mdo
Copy link
Member Author

mdo commented Aug 3, 2020

And maybe use normal font-size? Not sure what the rationale behind this choice was.

I knocked down the font-size to ideally help with readability. Bump it up in the inspector and see how it feels for you. For me it really helped with reading a table to downsize the font vs the body text. These tables are super content dense as well, and my secondary goal is to show more table contents to make it easier to scan the whole thing.

@Johann-S
Copy link
Member

Johann-S commented Aug 3, 2020

@rohit2sharma95 summarized everything here: #30838 (comment)
so you can use it @mdo

@XhmikosR
Copy link
Member

XhmikosR commented Aug 3, 2020

It just feels weird not having the tables responsive though. font-size might be objective :)

@mdo
Copy link
Member Author

mdo commented Aug 3, 2020

It just feels weird not having the tables responsive though.

Oh sorry, oversight on my part. I'll revisit and update.

@XhmikosR
Copy link
Member

XhmikosR commented Oct 2, 2020

After we merge #31803, I can switch to responsive tables in this branch, assuming we no longer use raw HTML tables.

@XhmikosR XhmikosR added this to Inbox in v5.0.0-alpha3 via automation Oct 2, 2020
@mdo mdo removed this from Inbox in v5.0.0-alpha3 Oct 28, 2020
@XhmikosR
Copy link
Member

XhmikosR commented Oct 31, 2020

@mdo I rebased this and switched to responsive tables too. Needs a thorough review just in case I missed something in the rebase (we should also check the whole docs again for more instances).

Otherwise, I'd say we can proceed with this.

EDIT: it seems we need to tweak the docs CSS for responsive tables to work properly.

@XhmikosR XhmikosR force-pushed the js-docs-tweaks branch 2 times, most recently from 785d15a to 8b864b2 Compare November 3, 2020 11:01
@GeoSot GeoSot added this to Inbox in v5.1.0 via automation Jun 29, 2021
@mdo mdo removed this from Inbox in v5.1.0 Jul 27, 2021
@mdo mdo added this to In progress in v5.2.0 via automation Jul 27, 2021
@XhmikosR XhmikosR force-pushed the js-docs-tweaks branch 3 times, most recently from 8146d96 to cd6776b Compare August 23, 2021 15:31
@XhmikosR XhmikosR requested review from ffoodd and removed request for Johann-S December 10, 2021 20:15
@XhmikosR
Copy link
Member

@ffoodd what's your take on this? Mostly the CSS stuff.

I say we land this as is and we migrate any missing components later, otherwise we'll have this hanging forever.

@ffoodd
Copy link
Member

ffoodd commented Dec 23, 2021

I think it needs some tweaks, e.g. the content table doesn't overflow properly and makes body overflow.

Not sure what happens for now.

@XhmikosR
Copy link
Member

XhmikosR commented Mar 1, 2022

@mdo @ffoodd should we land this after a rebase? We can't keep rebasing it :/

We just need some help with the CSS comments.

@GeoSot
Copy link
Member

GeoSot commented Mar 2, 2022

I think it needs some tweaks, e.g. the content table doesn't overflow properly and makes body overflow.

Wasn't wrapped with .table-responsive.

Added some more, and wrapped these that use html customization inside.

@XhmikosR XhmikosR force-pushed the js-docs-tweaks branch 2 times, most recently from fac3f30 to db44d9a Compare March 9, 2022 15:40
@XhmikosR XhmikosR moved this from In progress to Review in progress in v5.2.0 Mar 9, 2022
@XhmikosR XhmikosR marked this pull request as ready for review March 9, 2022 15:51
@XhmikosR XhmikosR requested a review from a team as a code owner March 9, 2022 15:51
Co-Authored-By: XhmikosR <xhmikosr@gmail.com>
Co-Authored-By: GeoSot <geo.sotis@gmail.com>
@mdo
Copy link
Member Author

mdo commented Mar 13, 2022

Updated this with some new copy changes to clean up <br> usage in a few JS tables and redo the containers and contents tables in Markdown. There are now no more responsive tables in our docs not using the table shortcode.

v5.2.0 automation moved this from Review in progress to Reviewer approved Mar 13, 2022
Copy link
Member

@ffoodd ffoodd left a comment

Choose a reason for hiding this comment

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

Nice work! This'll be really helpful 🚀

@XhmikosR
Copy link
Member

Let's land this and revisit any remaining instances later :)

@XhmikosR XhmikosR merged commit a9a89de into main Mar 14, 2022
v5.2.0 automation moved this from Reviewer approved to Done Mar 14, 2022
@XhmikosR XhmikosR deleted the js-docs-tweaks branch March 14, 2022 07:38
@louismaximepiton louismaximepiton mentioned this pull request Mar 31, 2022
@cccabinet cccabinet mentioned this pull request May 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
v5.2.0
  
Done
Development

Successfully merging this pull request may close these issues.

Be consistent with API tables
6 participants