[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
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

Rerender new icons #8

Merged
merged 6 commits into from
Aug 23, 2022
Merged

Rerender new icons #8

merged 6 commits into from
Aug 23, 2022

Conversation

adamwight
Copy link
Collaborator

Render fresh raster files for symbol and alphanumeric icons.

Base automatically changed from fork-to-wikimedia to master August 23, 2022 10:05
@adamwight adamwight changed the base branch from master to aliases August 23, 2022 10:15
Base automatically changed from aliases to master August 23, 2022 10:23
Copy link
Collaborator
@thiemowmde thiemowmde left a comment

Choose a reason for hiding this comment

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

I carefully reviewed every commit in this pull request and believe I can confirm everything. Note: There are two commits in here that are impossible to review via the GitHub UI. It just freezes. One way to avoid this in the future is to split this into two pull requests, one that updates all code and another that uploads all the new .png renders.

index.js Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved
return {
h: [hsl[0], hsl[0]],
s: [hsl[1], hsl[1]],
l: [hsl[2], hsl[2]],
Copy link
Collaborator

Choose a reason for hiding this comment

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

I had a little fun playing around with this.

const hsl = blend.rgb2hsl( r, g, b ).map( ( v ) => [ v, v ] );
return { h: hsl[ 0 ], s: hsl[ 1 ], l: hsl[ 2 ] };

Or even:

const hsl = blend.rgb2hsl( r, g, b );
return Object.fromEntries( hsl.map( ( v, i ) => [ 'hsl'[ i ], [ v, v ] ] ) );

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Neither feels more readable, although the exercise is fun!

Copy link
Collaborator

Choose a reason for hiding this comment

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

Sure, this wasn't about readability. 😜

index.js Outdated Show resolved Hide resolved
* without the bad contrast math.
*
* @param {string} color 6-digit hex color
* @returns {object} HSL with [0-1] float components. Each term is a redundant
Copy link
Collaborator

Choose a reason for hiding this comment

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

Type could be something like @return {Object.<string,number[]>} here, I think.

Also should use @return without the s.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I don't think the more detailed type improves readability, though?

@returns seems to be the canonical name: https://jsdoc.app/tags-returns.html and it's only the wikimedia eslint plugin that enforces @return. On that topic, maybe we should turn on a linter, allow ES6, and rewrite var for example?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Our canon is what the Wikimedia linter says. And we are in a Wikimedia codebase here, aren't we? But maybe this is an exception because it's a fork from something outside of the org. Let's not touch code style details for now.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

+1 that we've restyled repos in the past. Also +1 that it's a bigger job and best done by eslint --fix

index.js Show resolved Hide resolved
The old (maki 0.5.0) icons were rendered with a large margin which is
wasted space in a bitmap.  Instead, this patch renders the full
15x15 (units) new icons in smaller boxes with no margin.  Exact sizes
were chosen such that new icons are roughly congruent to the old ones.

There is a slight savings in the number of bits for each image.

Includes a few more symbols.
@adamwight adamwight force-pushed the rerender-new-icons branch 3 times, most recently from f0c3d93 to 8393ff4 Compare August 23, 2022 11:21
This still builds static PNGs to serve, but the source step is done
by a script.

No longer paints a stroke around glyphs, to match the new Maki style
guidelines.

Removes the deprecated alphanum master doc.
…a threshold

This makes the symbol visible on light backgrounds.
Was missing a color space conversion.  The resulting formula has been
checked against other references.
Calculate the luminance threshold dynamically, improving code
maintainability in case we need to change the color again.
@thiemowmde thiemowmde merged commit 82f118c into master Aug 23, 2022
@thiemowmde thiemowmde deleted the rerender-new-icons branch August 23, 2022 13:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants