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

Refactor icon generator (i.e. VueSimpleIcon component) #20

Merged
merged 12 commits into from Dec 25, 2018
Merged

Conversation

@ericcornelissen
Copy link
Contributor

@ericcornelissen ericcornelissen commented Dec 23, 2018

Description

The primary change is reducing the code complexity of and improving the performance when generating icons utilizing the domProps property to inject the SVG as it is provided by SimpleIcons. The <svg> itself is still created as a VNode in order to keep the size changes easy, but the contents of the SVG come straight from icon.svg.

The second change is the introduction of the title argument to the renderError() function, which makes it easier to render errors with a custom message/title.

Other changes:

  • Rename h to createElement for clarity.
  • Rename x in the name property validator to name for clarity.
  • Removed this.parser from the VueSimpleIcon class, it is no longer needed as per the primary change.
  • Use string literals rather then string concatenation (e.g. "#" + this.color --> #${this.color}).

Possible improvements:

  • Remove </svg> using a fixed method (e.g. String.substr) rather then a Regular Expression for performance. However, it is rather convenient to have the Regular Expression remove both the opening and closing SVG tag.
  • Inject custom titles in another way not using Regular Expressions to increase performance. However, this would require removing <title>...</title> from the icon.svg string which would probably still require a Regular Expression.

Why is this change required?

Should improve code readability and performance, not critical though.

  • I've read the contributing guidelines and the Code of Conduct
@codecov
Copy link

@codecov codecov bot commented Dec 23, 2018

Codecov Report

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

Impacted file tree graph

@@          Coverage Diff          @@
##           master    #20   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files           1      1           
  Lines          50     41    -9     
  Branches       16     11    -5     
=====================================
- Hits           50     41    -9
Impacted Files Coverage Δ
src/VueSimpleIcon.ts 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 8826b4f...0911c2f. Read the comment docs.

src/VueSimpleIcon.ts Outdated Show resolved Hide resolved
@sh7dm sh7dm self-requested a review Dec 24, 2018
@sh7dm
sh7dm approved these changes Dec 24, 2018
Copy link
Owner

@sh7dm sh7dm left a comment

Looks good!

@sh7dm sh7dm merged commit ab0b631 into master Dec 25, 2018
4 checks passed
4 checks passed
@codecov
codecov/patch 100% of diff hit (target 100%)
Details
@codecov
codecov/project 100% (+0%) compared to 8826b4f
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
@sh7dm
Copy link
Owner

@sh7dm sh7dm commented Dec 25, 2018

@ericcornelissen Thanks for these improvements!

@sh7dm sh7dm deleted the refactor-component branch Dec 25, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants