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

chore: switch two-column list styles to be opt-in #5110

Merged
merged 8 commits into from
Jun 19, 2024

Conversation

marjys
Copy link
Contributor

@marjys marjys commented Mar 3, 2024

PR Checklist

Overview

This PR:

  • Adds the two-column class with the expected style for two-column unordered lists, making sure to keep the two-column style at Features and Table of Contents
  • Removes column style from ul element scope (unneeded style)

- Adds the two-column class with the expected style for two-column unordered lists, making sure to keep the two-column style at Features and Table of Contents
- Removes column style from ul element scope
Copy link

linux-foundation-easycla bot commented Mar 3, 2024

CLA Signed

The committers listed above are authorized under a signed CLA.

@JoshuaKGoldberg JoshuaKGoldberg changed the title style: closes mochajs#3702 chore: switch two-column list styles to be opt-in Mar 4, 2024
Copy link
Member

@JoshuaKGoldberg JoshuaKGoldberg left a comment

Choose a reason for hiding this comment

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

Functionally looks good, nicely done! 🎨

Requesting changes on a bit of refactoring to make the styles more clear.

docs/css/style.css Outdated Show resolved Hide resolved
docs/css/style.css Outdated Show resolved Hide resolved
@JoshuaKGoldberg JoshuaKGoldberg added the status: waiting for author waiting on response from OP - more information needed label Mar 4, 2024
@marjys marjys force-pushed the repo/default-list-style branch 2 times, most recently from 92c6187 to c358aae Compare March 6, 2024 00:50
Use the class two-column instead of applying the style by using its id.
Move margin-top and padding inside two-column as they are overwritten
within single-column.
@marjys
Copy link
Contributor Author

marjys commented Mar 6, 2024

@JoshuaKGoldberg The 'Deploy Preview' check has failed even before I committed the requested changes (when I merged master to my branch), but I'm not authorized to open Netlify to check the error. Is there another way for me to verify that? Thanks!

@JoshuaKGoldberg
Copy link
Member

...huh, that's a new one.

8:01:04 PM: 2024-03-06T01:01:04.355Z mocha:docs:data:supporters supporter image pull completed
8:01:04 PM: [11ty] Problem writing Eleventy templates:
8:01:04 PM: [11ty] 1. Having trouble rendering liquid template ./docs/api/reporters_base.js.html (via TemplateContentRenderError)
8:01:04 PM: [11ty] 2. unexpected token at ": string, msg...", file:./docs/api/reporters_base.js.html, line:273, col:12 (via ParseError)
8:01:04 PM: [11ty] 3. unexpected token at ": string, msg..." (via AssertionError)
8:01:04 PM: [11ty] Wrote 0 files in 3.01 seconds (v1.0.2)
8:01:04 PM: The script called "docs" which runs "nps docs.clean && nps docs.api && eleventy && nps docs.linkcheck" failed with exit code 1 https://github.com/sezna/nps/blob/master/other/ERRORS_AND_WARNINGS.md#failed-with-exit-code

Does that happen for you? I'm traveling right now and blocked from trying it locally on #5113.

@marjys
Copy link
Contributor Author

marjys commented Mar 7, 2024

I can see this error locally now! It seems to be happening due to Eleventy not being able to parse the properties of an object on the Jsdocs api at /lib/reporters/base.js, line 230 (didn't catch it before as I was running npm start docs.watch and it does not build before serving, so I was serving an old version of my build, my fault).
I was thinking about some ways to fix this:

  1. Simply replace { message: string, msg: string, stack: string } with Object
  2. As we know what the Object structure is, we could add a type definition to it with something like the following, and then replacing the object with FullErrorStack:
 * @return {FullErrorStack}

...

/**
 * An object containing the full error stack results
 * @private
 * @typedef {Object} FullErrorStack
 * @property {string} message
 * @property {string} msg
 * @property {string} stack
 */

So please let me know your preferences and whether we should be opening those changes in another PR or if we could just apply them on this one.
Thanks!

@JoshuaKGoldberg
Copy link
Member

Nice investigation! 👏

Does that happen for you on the master branch? If does (i.e. is not unique to this PR) then yeah, that'd be a separate issue & PR. Yes please!

If it's unique to this branch (?!) then it'd be something to fix here.

@marjys
Copy link
Contributor Author

marjys commented Mar 7, 2024

The error happens on master too:

Screenshot 2024-03-07 at 11 29 39

Ok, I'll be opening an issue for this. Thanks a lot!

@marjys
Copy link
Contributor Author

marjys commented Mar 13, 2024

Requested changes were applied and all checks are passing after fix on #5116.

Copy link
Member

@JoshuaKGoldberg JoshuaKGoldberg left a comment

Choose a reason for hiding this comment

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

Michael Fassbender calmly saying "perfection"

@JoshuaKGoldberg JoshuaKGoldberg removed the status: waiting for author waiting on response from OP - more information needed label Jun 19, 2024
@JoshuaKGoldberg JoshuaKGoldberg merged commit e030115 into mochajs:master Jun 19, 2024
4 checks passed
@JoshuaKGoldberg
Copy link
Member

Thanks @marjys!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

🛠️ Repo: Two-column lists in the docs should not be the default style for lists
2 participants