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

Add block list for known conflicting root class names #211

Merged
merged 4 commits into from
Dec 8, 2023
Merged

Add block list for known conflicting root class names #211

merged 4 commits into from
Dec 8, 2023

Conversation

angelogladding
Copy link
Collaborator

Closes #170.

I'm seeing h-px, h-auto, h-full, h-screen, h-min, h-max, h-fit in the Tailwind docs. None of these seem to be good candidates for root class names so there shouldn't be an issue of vocabulary gatekeeping.

The general feeling I get from microformats/microformats2-parsing#59 is that parser experimentation would be a good path forward. This change can easily be reverted at a later date.

In the meantime we have a fix for the current landscape and a framework for solving similar issues moving forward.

@sknebel
Copy link
Member

sknebel commented Nov 12, 2023

This should be behind a flag IMHO.

@angelogladding
Copy link
Collaborator Author

Or should there be a flag to turn it off? It'll do only good right now and if someone somewhere wants to experiment with e.g. h-screen they can use the flag. Consumers of their h-screen will have to explicitly use the flag as well but their consumption of a new microformat would probably already need other manual steps to support.

@sknebel
Copy link
Member

sknebel commented Nov 12, 2023

I strongly believe default behavior of microformats parsers should be as similar as possible. Parser bugs already regularly cause confusion when someone checks their pages with one parser and then is confused why an application using a different parser does not understand it correctly.

@gRegorLove
Copy link
Member

Agreed, I think it should be behind a flag and (probably) configurable, like an option to initialize the parser with a custom list of class names to block.

This would allow a specialized app like indiewebify.me to choose to ignore any conflicting classes that arise over time, without requiring an update to the list of static class names in the parser.

@angelogladding
Copy link
Collaborator Author

@sknebel @gRegorLove what's a good flag? leave the tailwind-specific classes out of the library entirely and just let the user bring their own block list?

>>> parser(url="example.com", block_roots={
...     "h-px", "h-auto", "h-full", "h-screen", "h-min", "h-max", "h-fit"
... })

@gRegorLove
Copy link
Member

If it's behind a flag I don't have a strong opinion about including Tailwind classes in the package. If you do, maybe it could have three states:

  • default (no blocked classes)
  • block roots enabled (packaged list of classes is blocked)
  • block roots enabled + custom list of classes provided (only the custom list of classes is blocked)

@sknebel
Copy link
Member

sknebel commented Nov 15, 2023

One way might be to have the list be user-provided, but have a ready-to-use list as a constant available. filter_roots=mf2py.ROOT_FILTER_PRESET_TAILWIND ?

`filter_roots` defaults to False. Set to True to filter known
conflicting Tailwind classes. Otherwise provide a collection of custom
classes to filter.
@angelogladding
Copy link
Collaborator Author

angelogladding commented Nov 20, 2023

It's tempting to try to leverage this function https://github.com/microformats/mf2py/blob/main/mf2py/mf2_classes.py#L9 but it wasn't the right place to reject items.

As implemented, this method will only work on root (h-) classes. This works for the current Tailwind use case but won't extend to future elemental (u-, dt-, p-, e-) conflicts.

@angelogladding angelogladding mentioned this pull request Nov 30, 2023
Copy link
Member

@tantek tantek left a comment

Choose a reason for hiding this comment

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

This seems worth experimenting with a flag as commenters have said. LGTM.

@angelogladding angelogladding merged commit 34841b1 into microformats:main Dec 8, 2023
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.

Ignore Tailwind's h- prefix values
4 participants