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

Simplify VALID_TAG_NAMES definition #603

Merged
merged 1 commit into from
Jun 30, 2020

Conversation

chrissantamaria
Copy link
Contributor

Hi all 👋 this is a small one, just cleans up the definition of the VALID_TAG_NAMES array. Not sure if there's a reason to use Object.keys if the goal is to get an array of values anyways.

@codecov
Copy link

codecov bot commented Jun 29, 2020

Codecov Report

Merging #603 into master will decrease coverage by 0.01%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #603      +/-   ##
==========================================
- Coverage   96.91%   96.90%   -0.02%     
==========================================
  Files           3        3              
  Lines         292      291       -1     
==========================================
- Hits          283      282       -1     
  Misses          9        9              
Impacted Files Coverage Δ
src/HelmetConstants.js 100.00% <100.00%> (ø)

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 a564fb0...0139853. Read the comment docs.

@cwelch5
Copy link
Contributor

cwelch5 commented Jun 30, 2020

Thanks, that's a good call.

@cwelch5 cwelch5 merged commit 1b57ddb into nfl:master Jun 30, 2020
@realityking
Copy link
Contributor

I don't have a problem with this but note that Object.values requires either somewhat recent browser (e.g. no IE11) or a polyfill. Source: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_objects/Object/values

I think that's fine since most folks already use something like core-js but it should be documented. It also means the dependency on object-assign could be dropped since every browser that supports Object.values will also support Object.assign.

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.

3 participants