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.

Sign up for GitHub

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
Open

Prevent border shorthand when individual property isnt shortened #124

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
c7f3aec
Prevent border shorthand when individual property isnt shortened
kalilz4485 Feb 9, 2021
2a34af5
rubocop
kalilz4485 Feb 9, 2021
ce253ea
rubocop2
kalilz4485 Feb 9, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions lib/css_parser/rule_set.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -512,6 +512,11 @@ def create_background_shorthand! # :nodoc:
def create_border_shorthand! # :nodoc:
values = []

# can't merge if not shortened
declarations.each do |name, _value|
return nil if %w[border-top border-right border-bottom border-left].any? { |e| name.include?(e) }
end
Comment on lines +516 to +518
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 ?)


BORDER_STYLE_PROPERTIES.each do |property|
next unless (declaration = declarations[property])
next if declaration.important
Expand Down
10 changes: 10 additions & 0 deletions test/test_rule_set_creating_shorthand.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,16 @@ def test_combining_borders_into_shorthand
}
combined = create_shorthand(properties)
assert_equal '#bada55 #000000 #ffffff #ff0000;', combined['border-color']

# should not combine if individual side property is set
properties = {
'border-top-style' => 'none',
'border-width' => '1px',
'border-style' => 'solid',
'border-color' => 'black'
}
combined = create_shorthand(properties)
assert_equal '', combined['border']
end

# Dimensions shorthand
Expand Down