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

Prevent border shorthand when individual property isnt shortened #124

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

kalilz4485
Copy link

Issue #87

Checks if any border-side declaration is present, as it should have been removed by create_dimensions_shorthand! , if there is still one left it means it's not shortable and we shouldn't replace any border-width/style/color property with a border property that would erase it.

Open for suggestions/corrections

Pre-Merge Checklist

  • CHANGELOG.md updated with short summary

Comment on lines +516 to +518
declarations.each do |name, _value|
return nil if %w[border-top border-right border-bottom border-left].any? { |e| name.include?(e) }
end
Copy link
Contributor

Choose a reason for hiding this comment

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

something like this ?

Suggested change
declarations.each do |name, _value|
return nil if %w[border-top border-right border-bottom border-left].any? { |e| name.include?(e) }
end
return nil if (declarations.keys & BORDER_STYLE_PROPERTIES).any?

Copy link
Author

Choose a reason for hiding this comment

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

Mh I'm not sure to follow, the idea I had in mind was preventing from merging BORDER_STYLE_PROPERTIES together into a new border property that would be appended to the declarations, hence overwriting all other properties (even if a border-top/left/bottom/right was initially set after any BORDER_STYLE_PROPERTIES )

Your suggestion would abort if there is any BORDER_STYLE_PROPERTIES but we're supposed to iterate on them after so it's completely breaking it no ? (Or maybe it was just an example of structure you prefer and what you meant is actually to move the array in a new constant and add a .keys method to declarations to get all the declaration names)

Copy link
Contributor

Choose a reason for hiding this comment

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

declarations is a hash, so if I want to check if border-top is defined in that hash I can either do hash.each { |k,v| k.include? "border-top" } which was the initial commit
or: hash.each { |k,v| k == "border-top" }
or: hash.key? "border-top"

also the list of values this was checking looks like BORDER_PROPERTIES, so I suggested replacing that
(but did a mistake and used BORDER_STYLE_PROPERTIES ... so should be a new BORDER_DIRECTIONS constant ?)

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