Skip to content

Conversation

sn3p
Copy link
Collaborator

@sn3p sn3p commented Nov 6, 2017

Closes #9

@sn3p sn3p requested a review from jessedijkstra November 6, 2017 11:50
@codecov-io
Copy link

codecov-io commented Nov 6, 2017

Codecov Report

Merging #10 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@          Coverage Diff          @@
##           master    #10   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files           2      2           
  Lines          25     27    +2     
=====================================
+ Hits           25     27    +2
Impacted Files Coverage Δ
lib/ex_css_modules/view.ex 100% <ø> (ø) ⬆️
lib/ex_css_modules/ex_css_modules.ex 100% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0b636f3...805045d. Read the comment docs.

@sn3p sn3p added the ready label Nov 6, 2017
@sn3p sn3p force-pushed the matthijs/class-conditional branch from d1909d0 to 857ce69 Compare November 6, 2017 11:56
@sn3p sn3p force-pushed the matthijs/class-conditional branch from 857ce69 to 1f7e241 Compare November 6, 2017 11:57
@sn3p sn3p changed the title Ass class/2 for conditional checking Add class/2 for conditional checking Nov 6, 2017
nil
"""
def class(definition, classes, value) do
if value do
Copy link
Contributor

Choose a reason for hiding this comment

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

You're reproducing existing functionality because class_name already checks the value for truthiness.

iex(1)> ExCSSModules.class_name(
...(1)> %{ "hello" => "world" }, "hello", false)
nil
iex(2)> ExCSSModules.class_name(%{ "hello" => "world" }, "hello", false)
nil
iex(3)> ExCSSModules.class_name(%{ "hello" => "world" }, "hello", true)
"world"
iex(4)> ExCSSModules.class_name(%{ "hello" => "world", "world" => "hello" }, ["world"])
"hello"
iex(5)> ExCSSModules.class_name(%{ "hello" => "world", "world" => "hello" }, [{"world", false}])
""
iex(6)> ExCSSModules.class_name(%{ "hello" => "world", "world" => "hello" }, [{"world", false}])

Looking at these results I would do the following:
Change the class_name() function that reacts to lists to return nil instead of an empty string when no results are present, ie:

iex(5)> ExCSSModules.class_name(%{ "hello" => "world", "world" => "hello" }, [{"world", false}])
nil

And change the class attribute to return an empty string if it receives nil:

iex> class_attribute(nil)
""

This way the class function behaves the exact same way as a class_name except that it pipes to the class_attribute() function.

Copy link
Contributor

@jessedijkstra jessedijkstra left a comment

Choose a reason for hiding this comment

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

The class function duplicates behaviour which can create unwanted results if we ever change any of the behaviours.

) == "world"
end

test "returns nil ...." do
Copy link
Contributor

@jessedijkstra jessedijkstra Nov 6, 2017

Choose a reason for hiding this comment

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

Why is this appended with ....?

Should probably read returns nil if none of the conditions are met

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

woops

|> Enum.map(&class_name(definition, &1))
|> Enum.reject(&is_nil/1)

if Enum.any?(list) do
Copy link
Contributor

Choose a reason for hiding this comment

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

Prefer to away from if statements as much as possible, ie:

defp join_class_name(list) when length(list) == 0, do: nil
defp join_class_name(list), do: Enum.join(list, " ")

Copy link
Contributor

Choose a reason for hiding this comment

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

This way we can just keep piping the values as well.

Copy link
Collaborator Author

@sn3p sn3p Nov 6, 2017

Choose a reason for hiding this comment

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

Kinda had the same thought, do you really think this is more clear?

Copy link
Contributor

@jessedijkstra jessedijkstra Nov 6, 2017

Choose a reason for hiding this comment

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

I don't know yet ;)

It is quite clear however when you read the join_class_name function what it does, gets rid of an if statement and allows you to keep piping the value to the next function which I like a lot.

definition
|> class_name(classes)
|> class_attribute
|> class_attribute()
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

@sn3p sn3p merged commit a267990 into master Nov 7, 2017
@sn3p sn3p deleted the matthijs/class-conditional branch November 7, 2017 17:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants