Skip to content

Redesign content rule to support all possible values#180

Merged
giraud merged 8 commits intogiraud:masterfrom
erykpiast:feature/counter
Feb 17, 2020
Merged

Redesign content rule to support all possible values#180
giraud merged 8 commits intogiraud:masterfrom
erykpiast:feature/counter

Conversation

@erykpiast
Copy link
Copy Markdown
Contributor

@erykpiast erykpiast commented Jan 20, 2020

It's, unfortunately, a breaking change. To support values other than strings, I had to introduce variant type, so existing rules need to be migrated like this:

-  contentRule("foo")
+  contentRule(`string("foo"))

Do you think that's okay? Can we afford another major version in such a short time? Maybe there is another way to model this?

If we eventually agree to break the API, does it make sense to rename contentRule to content? (and contentRules to contents I suppose). I was quite surprised that content function is not available, but I assume there was a good reason for this name. What was that?

Plus, I've added support for counter-* rules and matching functions.

@erykpiast erykpiast requested a review from giraud January 20, 2020 21:36
Comment thread src/Css_AtomicTypes.re
Comment thread src/Css_AtomicTypes.re Outdated
@giraud
Copy link
Copy Markdown
Owner

giraud commented Feb 4, 2020

Sorry, I had little time recently to work on bs-css. I'll review that pr in the next days

@giraud
Copy link
Copy Markdown
Owner

giraud commented Feb 4, 2020

If we eventually agree to break the API, does it make sense to rename contentRule to content? (and contentRules to contents I suppose). I was quite surprised that content function is not available, but I assume there was a good reason for this name. What was that?

There was - unfortunately - already a content alias in the global namespace. contentRule was used to introduce that rule and not break existing code.

@erykpiast
Copy link
Copy Markdown
Contributor Author

If we eventually agree to break the API, does it make sense to rename contentRule to content? (and contentRules to contents I suppose). I was quite surprised that content function is not available, but I assume there was a good reason for this name. What was that?

There was - unfortunately - already a content alias in the global namespace. contentRule was used to introduce that rule and not break existing code.

Alright, that makes sense. I can see there is an alias for a polymorphic variant. 👍

Comment thread src/Css.re
Comment thread src/Css.re
);

let contentRule = x => D("content", {j|"$x"|j});
let contentRule = x => D("content", string_of_content(x));
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

if possible, I'd like to inline the string_of_content

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This way there will be duplication as I use the function in contentRules as well. What do you propose then?

Copy link
Copy Markdown
Contributor Author

@erykpiast erykpiast Feb 9, 2020

Choose a reason for hiding this comment

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

Maybe I could put these functions (together with string_of_counter_*) into modules inside AtomicTypes? I can see you're avoiding this so far (like string_of_background), what was the reason of that?

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

just string_of_content. Css.re is already huge, I'm trying to not add useless code. In that case, creating a function doensn't add anything. For operations, it's needed because it's used in multiple locations

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

string_of_content is used twice as well. Am I missing something?

Comment thread src/Css.rei Outdated
Comment thread src/Css_AtomicTypes.rei Outdated
Comment thread src/Css_AtomicTypes.rei Outdated
@erykpiast
Copy link
Copy Markdown
Contributor Author

Thank you so much for such a detailed comments, @giraud!

@giraud giraud merged commit 1c26948 into giraud:master Feb 17, 2020
@giraud
Copy link
Copy Markdown
Owner

giraud commented Feb 17, 2020

Thank you

@erykpiast
Copy link
Copy Markdown
Contributor Author

@giraud Hey! When can I expect a new version in NPM registry? :)

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.

2 participants