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

Add the Visitor Counter Block. #14

Open
wants to merge 10 commits into
base: master
Choose a base branch
from

Conversation

pento
Copy link
Contributor

@pento pento commented Dec 22, 2018

Description

Fixes #1.

This PR is based on this earlier effort: WordPress/gutenberg@4426159

Additionally, it adds the following features:

  • Use a monospace font, for that LED counter feel.
  • Add a faint background 8 behind each digit, giving the appearance of an unlit LED digit.
  • Add an option to change how many digits the counter should be padded to.
  • Add screen reader text.

Screenshots

Screen shot of the visitor counter block in the editor.

Screen shot of the visitor counter block on the front end.

@melchoyce
Copy link
Owner

What do you think about one of these for the block icon?

https://material.io/tools/icons/?icon=trending_up&style=outline
https://material.io/tools/icons/?icon=add_box&style=outline
https://material.io/tools/icons/?icon=plus_one&style=outline

Also — any chance we can vertically center the numbers within the container?

@melchoyce melchoyce requested a review from ryelle December 22, 2018 17:29
@pento
Copy link
Contributor Author

pento commented Dec 23, 2018

Thank you for the suggestions, @melchoyce!

I used the trending_up icon. The fix for the bottom margin being too large was a little hacky, because line-height is weird.

While I was there, I added faded background digits, in the style of a digital clock. This did mean that I had to place each digit in its own <div>, to fake a monospace style (this is most obvious with the digit 1). This is wildly inaccessible, of course, so I've added screen reader text as an alternative. This seems to work as expected when testing in Voiceover.

I've updated the screenshots in the description to match these changes.

@pento
Copy link
Contributor Author

pento commented Dec 23, 2018

It occurred to me that there might be a Digital 7 Monospace font. There is, so I switched to that, and removed the fake monospace styling.

I left the screen reader text, as this is still a better option for screen readers than just the number: it provides context that sighted users gain from the visual styling.

@pento
Copy link
Contributor Author

pento commented Dec 23, 2018

I'm not wild about the wording of the Padding Digits help text (or the name, "Padding Digits", for that matter). They feel like they were written by a programmer. 😉

@melchoyce
Copy link
Owner

Looking good. I'm not totally sure what "padding digits" does — just inflate the size?

@pento
Copy link
Contributor Author

pento commented Dec 24, 2018

Yah, it adds (or removes) extra zeros when the counter isn't a large enough number to use the full space.

@melchoyce
Copy link
Owner

Does this need to be a setting, or can we just pick a random number of 0s?

@pento
Copy link
Contributor Author

pento commented Dec 24, 2018

It doesn't need to be a setting, it previously had a default of 8: I added the setting to see what it looked like, I'm fine with removing it, too. 🙂

@melchoyce
Copy link
Owner

Let's nix it 👍

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.

2 participants