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

Feat: fixTableCellAlign option #48

Merged
merged 3 commits into from
Apr 14, 2023
Merged

Feat: fixTableCellAlign option #48

merged 3 commits into from
Apr 14, 2023

Conversation

jorroll
Copy link
Contributor

@jorroll jorroll commented Apr 12, 2023

Initial checklist

  • I read the support docs
  • I read the contributing guide
  • I agree to follow the code of conduct
  • I searched issues and couldn’t find anything (or linked relevant results below)
  • If applicable, I’ve added docs and tests

Description of changes

Adds a new plugin option, mapDepricatedTableCellAttrsToInlineCssfixTableCellAlign, which controls whether table-related attributes are mapped to inline CSS styles. Defaults to true.

Closes #47

Notes:

  1. I created this using Github's online vscode editor (i.e. press . when viewing a repo) so I'm not sure if this conforms to any required lint/formatting rules.
  2. My own preference is to default to clear, verbose names, but I won't be offended if you'd prefer to rename this option to something else.

@github-actions github-actions bot added the 👋 phase/new Post is being triaged automatically label Apr 12, 2023
@github-actions

This comment has been minimized.

@jorroll
Copy link
Contributor Author

jorroll commented Apr 12, 2023

Looks like a ts-ignore which I removed was necessary after all. I'll fix and add some tests...

@jorroll jorroll marked this pull request as draft April 12, 2023 19:55
@jorroll jorroll force-pushed the patch-1 branch 2 times, most recently from f19c25d to 8e32806 Compare April 12, 2023 20:49
@github-actions github-actions bot added 🤞 phase/open Post is being triaged manually and removed 👋 phase/new Post is being triaged automatically labels Apr 12, 2023
@codecov-commenter
Copy link

codecov-commenter commented Apr 12, 2023

Codecov Report

Patch coverage: 100.00% and no project coverage change.

Comparison is base (497de53) 100.00% compared to head (e7e99bb) 100.00%.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

Additional details and impacted files
@@            Coverage Diff            @@
##              main       #48   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            2         2           
  Lines          117       129   +12     
=========================================
+ Hits           117       129   +12     
Impacted Files Coverage Δ
lib/index.js 100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@jorroll jorroll marked this pull request as ready for review April 12, 2023 20:51
@jorroll
Copy link
Contributor Author

jorroll commented Apr 12, 2023

Ready for review

Copy link
Member

@wooorm wooorm left a comment

Choose a reason for hiding this comment

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

Thanks! Mostly some stylistic suggestion, otherwise looks good!

lib/index.js Outdated Show resolved Hide resolved
lib/index.js Outdated
Comment on lines 23 to 26
* Default: true. Map deprecated table-related attributes to css styles as
* recommended by Mozilla Developer Network docs. E.g.
* `align="center"` to `style="text-align: center;"`. May result
* in rendering differences.
Copy link
Member

Choose a reason for hiding this comment

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

Some recommendations:

Suggested change
* Default: true. Map deprecated table-related attributes to css styles as
* recommended by Mozilla Developer Network docs. E.g.
* `align="center"` to `style="text-align: center;"`. May result
* in rendering differences.
* Fix obsolete align attributes on table cells by turning them into inline
* styles.
* Keep it on when working with markdown, turn it off when working with
* markup for emails.

Feel free to iterate on this.

  • Uses semantic line breaks
  • default: true is already added with the [xxx=true] notation
  • the goal of this feature is to solve obsolete attributes, not just because MDN says it too
  • adds a recommendation on when to have it on/off

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I accepted this change with the exception that I kept the Default: true comment at the end. When typescript processes this comment and creates the SharedOptions interface, it keeps the JSDoc comments but doesn't keep any reference to [fixTableCellAlign=true] (so, when inspecting the type in the editor, you don't know that it has a default value). By also referencing the default value in the comment, we ensure that the generated type retains this information.

For example, here's what I see when I inspect the prefix option in VS Code. There is no reference to a default value in the editor tooltip and there also isn't a reference to it in the generated typescript type.

Screenshot 2023-04-13 at 11 08 38 AM

Copy link
Member

Choose a reason for hiding this comment

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

I feel like this is a TypeScript bug.
As you show, it’s also in all the other options here.
And in all the other projects.

It doesn’t hurt too much to include this information if TypeScript doesn’t but then it should be a sentence, and also use `code`.

lib/index.js Outdated Show resolved Hide resolved
readme.md Outdated Show resolved Hide resolved
@jorroll jorroll force-pushed the patch-1 branch 3 times, most recently from 1033e03 to 16122fc Compare April 13, 2023 17:28
@jorroll jorroll changed the title Feat: mapDepricatedTableCellAttrsToInlineCss option Feat: fixTableCellAlign option Apr 13, 2023
Adds a new plugin option, `mapDepricatedTableCellAttrsToInlineCss`, which controls whether table-related attributes are mapped to inline CSS styles. Defaults to `true`.
@jorroll jorroll requested a review from wooorm April 13, 2023 17:53
@wooorm wooorm changed the title Feat: fixTableCellAlign option Feat: fixTableCellAlign option Apr 14, 2023
readme.md Outdated Show resolved Hide resolved
lib/index.js Outdated
Comment on lines 23 to 26
* Default: true. Map deprecated table-related attributes to css styles as
* recommended by Mozilla Developer Network docs. E.g.
* `align="center"` to `style="text-align: center;"`. May result
* in rendering differences.
Copy link
Member

Choose a reason for hiding this comment

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

I feel like this is a TypeScript bug.
As you show, it’s also in all the other options here.
And in all the other projects.

It doesn’t hurt too much to include this information if TypeScript doesn’t but then it should be a sentence, and also use `code`.

lib/index.js Outdated Show resolved Hide resolved
Signed-off-by: Titus <tituswormer@gmail.com>
Signed-off-by: Titus <tituswormer@gmail.com>
@wooorm wooorm merged commit b84c624 into rehypejs:main Apr 14, 2023
@github-actions

This comment has been minimized.

@wooorm wooorm added the 💪 phase/solved Post is done label Apr 14, 2023
@wooorm
Copy link
Member

wooorm commented Apr 14, 2023

Thanks, released in 7.2.0!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
💪 phase/solved Post is done
Development

Successfully merging this pull request may close these issues.

The align property is stripped from td elements, breaking styling
3 participants