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 support for the not prefix in media queries #564

Merged
merged 9 commits into from
Jan 10, 2024
Merged

Conversation

Paril
Copy link
Contributor

@Paril Paril commented Jan 5, 2024

Adds basic support for the not prefix in media queries. I required this for my project as I wanted to construct menus with all potential elements added beforehand, semantically getting disabled via media queries if they don't currently apply to the situation (for instance, options menus may hide or show options depending on specific platforms, gyro availability, etc).

…support for `not()` (a single feature cannot be negated, only the entire query)
Copy link
Owner

@mikke89 mikke89 left a comment

Choose a reason for hiding this comment

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

Hey, and thanks for the pull request. I like the addition!

Please take a look at the comments. I would also appreciate it if you could add some unit tests, especially for the new feature. But I see that we could also improve the existing tests. I have some examples in my comments that we should probably include in our testing.

Source/Core/StyleSheetParser.h Outdated Show resolved Hide resolved
Include/RmlUi/Core/StyleSheetTypes.h Outdated Show resolved Hide resolved
Source/Core/StyleSheetParser.cpp Outdated Show resolved Hide resolved
Source/Core/StyleSheetParser.cpp Outdated Show resolved Hide resolved
Source/Core/StyleSheetParser.cpp Show resolved Hide resolved
@mikke89 mikke89 added the enhancement New feature or request label Jan 6, 2024
@Paril
Copy link
Contributor Author

Paril commented Jan 7, 2024

Addressed a few of the comments - positive unit tests are here, stronger types are being used. I don't have any negative unit tests yet for testing error cases - I skimmed the existing tests and couldn't find any that explicitly checked for parsing warnings/errors so I don't really know how I'd implement that other than just checking that the query was discarded (its block doesn't match anything) I guess?

@Paril
Copy link
Contributor Author

Paril commented Jan 7, 2024

Oh, I also just noticed that most of the parsers don't even check their return values, so they end up adding in broken media blocks... for instance if ParseDecoratorBlock or ParseMediaFeatureMap returns false they just keep the block with whatever happens to be in it at the time. Is that an oversight?

Include/RmlUi/Core/StyleSheetTypes.h Outdated Show resolved Hide resolved
Source/Core/StyleSheetParser.cpp Show resolved Hide resolved
Source/Core/StyleSheetParser.h Outdated Show resolved Hide resolved
Source/Core/StyleSheetParser.cpp Show resolved Hide resolved
Co-authored-by: Michael R. P. Ragazzon <[email protected]>
Copy link
Owner

@mikke89 mikke89 left a comment

Choose a reason for hiding this comment

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

Nice, this looks very good. I only have a few minor comments left, but it should be very close to merging.

I see that you found the way to test for expected warnings, looks good to me.

Oh, I also just noticed that most of the parsers don't even check their return values, so they end up adding in broken media blocks... for instance if ParseDecoratorBlock or ParseMediaFeatureMap returns false they just keep the block with whatever happens to be in it at the time. Is that an oversight?

We generally don't try to go out of our way to try to correct bad documents and styles. The main emphasis is rather on providing a helpful warning message that users can use to fix up their document. And of course, as long as we don't do anything highly unexpected or crashing.

@mikke89 mikke89 merged commit 4c7ba7a into mikke89:master Jan 10, 2024
18 checks passed
@mikke89
Copy link
Owner

mikke89 commented Jan 10, 2024

Looks great, thanks for the contribution! :)

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

Successfully merging this pull request may close these issues.

2 participants