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

feat(icon): add support for icon purpose #246

Merged
merged 14 commits into from Mar 12, 2020
Merged

feat(icon): add support for icon purpose #246

merged 14 commits into from Mar 12, 2020

Conversation

hans00
Copy link
Contributor

@hans00 hans00 commented Oct 28, 2019

@codecov
Copy link

codecov bot commented Oct 28, 2019

Codecov Report

Merging #246 into dev will increase coverage by 0.64%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##              dev     #246      +/-   ##
==========================================
+ Coverage   73.81%   74.46%   +0.64%     
==========================================
  Files          11       11              
  Lines         317      325       +8     
  Branches       98      102       +4     
==========================================
+ Hits          234      242       +8     
  Misses         72       72              
  Partials       11       11              
Impacted Files Coverage Δ
lib/icon/module.js 94.28% <100.00%> (+0.73%) ⬆️

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 8214d8f...1dc3b1f. Read the comment docs.

Copy link
Member

@pi0 pi0 left a comment

Thanks!

**purpose**
- Default: null

Array of icon purpose.
Copy link
Member

@pi0 pi0 Oct 28, 2019

Choose a reason for hiding this comment

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

Can you please improve docs by putting more description or links?

Copy link
Contributor Author

@hans00 hans00 Nov 13, 2019

Choose a reason for hiding this comment

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

I added some description, please review.

lib/icon/module.js Outdated Show resolved Hide resolved
@hans00 hans00 requested a review from ricardogobbosouza Nov 13, 2019
Copy link
Member

@ricardogobbosouza ricardogobbosouza left a comment

Hi @hans00, sorry for the late review.
Another note, you need to include tests for this feature.

lib/icon/module.js Show resolved Hide resolved
@hans00 hans00 requested a review from ricardogobbosouza Dec 6, 2019
Copy link
Member

@ricardogobbosouza ricardogobbosouza left a comment

Thanks @hans00
Sounds good, now we need tests

@hans00 hans00 requested a review from ricardogobbosouza Dec 7, 2019
@ulissesferreira
Copy link

ulissesferreira commented Dec 10, 2019

Hey guys, just wanted to say maskable icons are officially out what do we need to get this rolling? Can I help?

@ricardogobbosouza ricardogobbosouza requested a review from pi0 Dec 11, 2019
if (options.purpose) {
const purpose = Array.isArray(options.purpose) ? options.purpose : [options.purpose]
const validPurpose = ['badge', 'maskable', 'any']
options.purpose = purpose.filter(item => validPurpose.includes(item))
Copy link
Member

@pi0 pi0 Dec 16, 2019

Choose a reason for hiding this comment

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

Silently filtering invalid items is not a good idea. Maybe we can show a warning instead? (one idea can be keeping length before filter and warn if length changed)

Copy link
Member

@ricardogobbosouza ricardogobbosouza Dec 16, 2019

Choose a reason for hiding this comment

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

warn user is a good idea

Copy link
Member

@pi0 pi0 Mar 12, 2020

Choose a reason for hiding this comment

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

@@ -109,11 +117,15 @@ async function generateIcons (options) {
options._icons[size] = `${options.targetDir}/icon_${size}.${options.iconHash}.png`
}

// Generate _purpose
options._purpose = options.purpose ? options.purpose.join(' ') : undefined
Copy link
Member

@pi0 pi0 Dec 16, 2019

Choose a reason for hiding this comment

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

Do we need _purpose anywhere else? If not, can we just use const purpose = ...?

Copy link
Member

@pi0 pi0 Mar 12, 2020

Choose a reason for hiding this comment

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

@TomSmith27
Copy link

TomSmith27 commented Mar 1, 2020

Can someone merge this change in, as without this icons have a white background on some phones and don't look very good

@pi0 pi0 changed the title Add support for icon purpose feat(icon): add support for icon purpose Mar 12, 2020
@pi0 pi0 merged commit 9248174 into nuxt-community:dev Mar 12, 2020
3 checks passed
@pi0
Copy link
Member

pi0 commented Mar 12, 2020

I've fixed remaining tasks. Thanks @hans00 and @ricardogobbosouza ❤️

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.

None yet

5 participants