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

Render markdown tables in legislation draft #5136

Conversation

karim-semmoud
Copy link
Contributor

@karim-semmoud karim-semmoud commented Jun 22, 2023

Reference

Markdown not rendering correctly #5135

Objectives

This pull request aims at fixing the display of tables in the Legislation Draft from as a user, it also add test in order to make sure that the tables are rendered.

1- Allow table tags in Admin Legislation Sanitizer

2- Add Tables option to Redcarpet in Legislation draft
We used render_options and extensions variable in order to align with the existing code at app/helpers/application_helper.rb

3- Add custom CSS for Table inside Legislation draft using global table variables. we also make the table responsive with scrolling

4- Add Test to render markdown tables in Legislation drafts as a user and as an admin

5- Add Test for Admin Legislation Sanitizer. We also strengthen allowed and not allowed parameters

Visual Changes

Screenshot from 2023-06-26 18-11-49
Screenshot from 2023-06-26 18-11-03

Notes

@javierm javierm added this to Doing in Consul Democracy Jun 22, 2023
@karim-semmoud karim-semmoud marked this pull request as ready for review June 22, 2023 00:36
@javierm javierm moved this from Doing to Reviewing in Consul Democracy Jun 22, 2023
@taitus taitus self-assigned this Jun 26, 2023
Copy link
Member

@taitus taitus left a comment

Choose a reason for hiding this comment

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

Hi, @karim-semmoud

Thanks a lot for this pull request! 🎉

I've left a couple of comments. Let me know what you think!

@javierm javierm moved this from Reviewing to Doing in Consul Democracy Jun 26, 2023
@karim-semmoud
Copy link
Contributor Author

Hi @taitus
Thank you for the review and comments.

1 -How about using click_link "Launch text editor" instead of click_link class: "fullscreen-toggle" in this test?
I agree, I will make the changes

2 -At Consul we try to avoid putting comments in the code, the commit message might be a good place to add this reference to the options offered by RedCarpet thinking
My bad, I copied the code from app/helpers/application_helper.rb without removing the comment line.
I also removed the comment line from application_helper.rb if that is ok

3-Table CSS
I agree with you , it makes more sense and keep consistency in the code

I will make the changes necessary and commit them

Extra Note:
Regarding the CSS for the Legislation draft, we may need in the future to use custom CSS for all the element inside Legislation Draft.
If an installation uses "fancy" colors for the text, table, headings ( for example pink on black ), I don't think it is a good idea to reuse the same CSS inside the Legislation Draft as it is supposed to look neutral and "official".
Ideally we would set neutral and "official document "looking font family, size, colors, headings, tables, links etc ...
Currently we only set the background color to white, but use the main body color etc ...
But I guess this is a discussion to be continued on a different pull request

@javierm javierm added the 2.0 label Jun 27, 2023
@javierm javierm moved this from Doing to Reviewing in Consul Democracy Jun 29, 2023
@javierm
Copy link
Member

javierm commented Jun 29, 2023

I copied the code from app/helpers/application_helper.rb

This comment made me wonder why we've got two different ways of rendering markdown, with only one of them enabling certain extensions. Maybe it made sense before we stopped storing the generated HTML in the database but, if so, I think we can now unify the code 🤔.

@karim-semmoud @taitus Do you think it'd make sense? Here are a couple of commits that make the Legislation::DraftVersion model use the same code as the markdown helper. @karim-semmoud If you think this is a good idea, could you add them to this pull request (using git am <file>)?

0001-Use-the-same-extensions-in-all-markdown-renderers.txt
0002-Extract-markdown-helper-logic-to-a-class.txt

@javierm javierm moved this from Reviewing to Doing in Consul Democracy Jun 29, 2023
Copy link
Member

@javierm javierm left a comment

Choose a reason for hiding this comment

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

@karim-semmoud I've left a small comment; other than that, could you squash the first four commits of this pull request (all the commits except the ones I've posted in another comment) into one, since there are three commits that basically fix the first one?

Thanks a lot!

app/assets/stylesheets/legislation_process.scss Outdated Show resolved Hide resolved
@karim-semmoud karim-semmoud force-pushed the Legislation-draft-render-markdown-tables branch from 459fe42 to 1be2430 Compare June 29, 2023 18:37
karim-semmoud and others added 3 commits June 29, 2023 20:48
* Add Tables option to Redcarpet in Legislation draft

* Allow table tags in Admin Legislation Sanitizer

* Add Test to render markdown tables in Legislation drafts

* Add Test for Admin Legislation Sanitizer

We include test for image, table and h1 to h6 tags and additional tests to strengthen the allowed and disallowed parameters

* Add Table from markdown test in System and Factories

* Add test to render  tables for admin user

* Remove comment line about Redcarpet options

* Edit custom css for legislation draft table to make it responsive
We were using two different sets of extensions but, since the markdown
code is always written by administrators, IMHO it makes sense to be
consistent and always render markdown code the same way.
This way it'll be easier for other Consul installations to overwrite
parts of the code, like the default options.
@karim-semmoud karim-semmoud force-pushed the Legislation-draft-render-markdown-tables branch from 1be2430 to af618ea Compare June 29, 2023 18:49
@karim-semmoud
Copy link
Contributor Author

@javierm
I applied and pushed your 2 commits as you advised
I also squashed my previous commits into a single one
Let me know if there is anything else needed

Copy link
Member

@javierm javierm left a comment

Choose a reason for hiding this comment

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

@karim-semmoud Once again, thank you very much for your contributions!

Consul Democracy automation moved this from Doing to Testing Jun 29, 2023
@javierm javierm linked an issue Jun 29, 2023 that may be closed by this pull request
@javierm javierm merged commit a668ecd into consuldemocracy:master Jun 29, 2023
7 of 9 checks passed
Consul Democracy automation moved this from Testing to Release 2.0.0 Jun 29, 2023
@javierm javierm added Admin and removed 2.0 labels Jun 29, 2023
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.

Markdown not rendering correctly
3 participants