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

Test collapsibles #1149

Closed
wants to merge 3 commits into from
Closed

Test collapsibles #1149

wants to merge 3 commits into from

Conversation

advename
Copy link
Contributor

@advename advename commented Jun 18, 2023

Regarding #1101, we've been discussing adding Collapsibles to OWASP to add additional information without increasing the size of the cheatsheet.

After some research, there seems 2 approaches here:

  1. it seems that MKDocs, the framework of the cheatsheet, supports this but requires additional configuration. (See: https://squidfunk.github.io/mkdocs-material/reference/admonitions/)
  2. With HTML https://gist.github.com/pierrejoubert73/902cc94d79424356a8d20be2b382e1ab

I added test collapsibles at the end of the CSRF cheatsheet page to see how they render. We can remove this instantly afterward. I'd like to know if they render OK before I invest time in #1143 or #1101.

@advename advename requested a review from kwwall as a code owner June 18, 2023 11:39
@advename advename changed the title Add Admonitions config to mkdocs.yml Test collapsibles Jun 19, 2023
@advename
Copy link
Contributor Author

@jmanico or @mackowski, happy to move forward with this

@jmanico
Copy link
Member

jmanico commented Jun 25, 2023

This looks good, some lint issues but glad its working

@advename
Copy link
Contributor Author

@jmanico can we merge this to main and see how both collapsibles are rendered?

@tghosth
Copy link
Contributor

tghosth commented Jun 26, 2023

@advename @jmanico PR #1158 should sort out the lint issues

@advename
Copy link
Contributor Author

@thghosth, thanks! The CSRF Cheatsheet had already some linting errors, so I don't believe the check fails because of the new collapsible. @jmanico force merged it for my last PR.

@jmanico
Copy link
Member

jmanico commented Jun 30, 2023

@thghosth, thanks! The CSRF Cheatsheet had already some linting errors, so I don't believe the check fails because of the new collapsible. @jmanico force merged it for my last PR.

This was my mistake, my apologies for the early merge.

@advename
Copy link
Contributor Author

@jmanico how do you want to proceed? Those linting errors have been there for some time.

@tghosth
Copy link
Contributor

tghosth commented Jul 3, 2023

So @advename here is an example of how collapsibles using HTML renders:

https://cheatsheetseries.owasp.org/cheatsheets/Java_Security_Cheat_Sheet.html#symmetric-example-using-google-tink

Is that sufficient to help you decide what to do? Do you still need to merge this PR now?

@advename
Copy link
Contributor Author

advename commented Jul 3, 2023

@tghosth I see that you used the

<details>
  <summary>Click me</summary>
  ... stuff
</details>

Approach. It renders code correctly, but the text is quite small.
image

I'm OK with it, but would still be interested to see how the other syntax renders.

@kwwall
Copy link
Collaborator

kwwall commented Jul 3, 2023 via email

@advename
Copy link
Contributor Author

advename commented Jul 4, 2023

I think 'small(er)' is fine, especially for code blocks as it makes
horizontal scroll bars less needed.
Plus anyone reading these cheat sheets with such collapsible content almost
certainly knows about Ctrl-+ or whatever else their browser of choice uses
to zoom in.

I don't think that Collapsibles should become a dump yard for code blocks. Collapsibles, from the linked issue, should be used to reduce the size of the Cheatsheet to the minimum, with Collapsibles adding additional content (text, links & code blocks) that may explain why the previous is required, or where you can read more about the content.

Moreover, the Collapsibles content from that page has a font-size of 10px, which is quite low and makes things hard to read. From a UX perspective, we should not rely on users to use CTRL+ to make it more readable.

@mackowski
Copy link
Collaborator

@advename all linter errors are around your changes these are all new. You can verify that in two ways:

  1. Your changes are in lines 464-495
    Screenshot 2023-08-08 at 12 54 47
    The same lines where liner errors occurs https://github.com/OWASP/CheatSheetSeries/actions/runs/5303437837/job/14564834274?pr=1149:
> markdownlint ./cheatsheets/ --ignore node_modules
cheatsheets/Cross-Site_Request_Forgery_Prevention_Cheat_Sheet.md:465 MD012/no-multiple-blanks Multiple consecutive blank lines [Expected: 1; Actual: 2]
cheatsheets/Cross-Site_Request_Forgery_Prevention_Cheat_Sheet.md:472 MD046/code-block-style Code block style [Expected: fenced; Actual: indented]
cheatsheets/Cross-Site_Request_Forgery_Prevention_Cheat_Sheet.md:480:1 MD033/no-inline-html Inline HTML [Element: details]
cheatsheets/Cross-Site_Request_Forgery_Prevention_Cheat_Sheet.md:481:3 MD033/no-inline-html Inline HTML [Element: summary]
cheatsheets/Cross-Site_Request_Forgery_Prevention_Cheat_Sheet.md:483 MD022/blanks-around-headings/blanks-around-headers Headings should be surrounded by blank lines [Expected: 1; Actual: 0; Below] [Context: "### Heading"]
cheatsheets/Cross-Site_Request_Forgery_Prevention_Cheat_Sheet.md:483:1 MD023/heading-start-left/header-start-left Headings must start at the beginning of the line [Context: "  ### Heading"]
cheatsheets/Cross-Site_Request_Forgery_Prevention_Cheat_Sheet.md:484 MD032/blanks-around-lists Lists should be surrounded by blank lines [Context: "1. Foo"]
cheatsheets/Cross-Site_Request_Forgery_Prevention_Cheat_Sheet.md:486:1 MD004/ul-style Unordered list style [Expected: dash; Actual: asterisk]
cheatsheets/Cross-Site_Request_Forgery_Prevention_Cheat_Sheet.md:487:1 MD004/ul-style Unordered list style [Expected: dash; Actual: asterisk]
cheatsheets/Cross-Site_Request_Forgery_Prevention_Cheat_Sheet.md:489 MD0[22](https://github.com/OWASP/CheatSheetSeries/actions/runs/5303437837/job/14564834274?pr=1149#step:5:23)/blanks-around-headings/blanks-around-headers Headings should be surrounded by blank lines [Expected: 1; Actual: 0; Below] [Context: "### Some Code"]
cheatsheets/Cross-Site_Request_Forgery_Prevention_Cheat_Sheet.md:489:1 MD0[23](https://github.com/OWASP/CheatSheetSeries/actions/runs/5303437837/job/14564834274?pr=1149#step:5:24)/heading-start-left/header-start-left Headings must start at the beginning of the line [Context: "  ### Some Code"]
cheatsheets/Cross-Site_Request_Forgery_Prevention_Cheat_Sheet.md:490 MD031/blanks-around-fences Fenced code blocks should be surrounded by blank lines [Context: "```js"]
cheatsheets/Cross-Site_Request_Forgery_Prevention_Cheat_Sheet.md:494 MD031/blanks-around-fences Fenced code blocks should be surrounded by blank lines [Context: "```"]
  1. You can also check my test PR Update Cross-Site_Request_Forgery_Prevention_Cheat_Sheet.md #1180 for the same cheatsheet

@szh szh marked this pull request as draft August 30, 2023 13:46
@jmanico
Copy link
Member

jmanico commented Feb 2, 2024

politely closing this, I do not think we need it, respectfully.

@jmanico jmanico closed this Feb 2, 2024
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.

5 participants