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: improve card semantics (resolves #190) #209

Merged
merged 12 commits into from
Jan 30, 2020

Conversation

greatislander
Copy link
Collaborator

@greatislander greatislander commented Jan 28, 2020

Description

Applies changes from #190 to all cards.

TODO: Add icon to byline.

Steps to test

Review all card and archive components: https://deploy-preview-209--pinecone.netlify.com

Additional information

Not applicable.

Related issues

@greatislander greatislander self-assigned this Jan 28, 2020
@greatislander greatislander added the enhancement New feature or request label Jan 28, 2020
@greatislander greatislander added this to In progress in Pinecone 1.0.0 via automation Jan 28, 2020
@greatislander greatislander added this to To do in Cooperative Resource Library 1.0.0 via automation Jan 28, 2020
@greatislander greatislander added this to the 1.0.0-alpha.10 milestone Jan 28, 2020
@netlify
Copy link

netlify bot commented Jan 28, 2020

Deploy preview for pinecone ready!

Built with commit d958e44

https://deploy-preview-209--pinecone.netlify.com

@cherylhjli
Copy link

the page format looks great! the location looks like its hanging a bit lower than the other ones in the individual card component?

@greatislander
Copy link
Collaborator Author

greatislander commented Jan 28, 2020

Good catch @cherylhjli! Fixed now.

@greatislander greatislander marked this pull request as ready for review January 28, 2020 22:57
@greatislander
Copy link
Collaborator Author

@cherylhjli This is done! I just need the icon for the byline.

@greatislander greatislander moved this from To do to In progress in Cooperative Resource Library 1.0.0 Jan 28, 2020
@greatislander greatislander moved this from In progress to Review in progress in Cooperative Resource Library 1.0.0 Jan 28, 2020
@jhung
Copy link
Contributor

jhung commented Jan 29, 2020

I think the "+2 more" should be part of the Topics list. Right now "+2 more" appears out of context. For example, NVDA would say:

"... List item topic commons. List item topic policy. Out of list. Plus two more"

It may make more sense if the feedback says:

"... List item topic commons. List item topic policy. Plus two more topics. Out of list."

src/components/02-molecules/01-card/card--resource.njk Outdated Show resolved Hide resolved
src/components/02-molecules/01-card/card--resource.njk Outdated Show resolved Hide resolved
src/components/02-molecules/01-card/card--resource.njk Outdated Show resolved Hide resolved
src/components/02-molecules/01-card/card--resource.njk Outdated Show resolved Hide resolved
src/components/02-molecules/01-card/card--resource.njk Outdated Show resolved Hide resolved
Pinecone 1.0.0 automation moved this from In progress to Review in progress Jan 29, 2020
@greatislander
Copy link
Collaborator Author

@jhung I think I've addressed all the review comments (including adding screen reader-accessible labels to the other cards).

@jhung
Copy link
Contributor

jhung commented Jan 30, 2020

One last thing, the mid-dot does not cause the screen reader to pause. This makes "resource format article (mid-dot) publisher" come out as a single uninterrupted string of words which is confusing.

I tried to experiment with this by adding aria-label="." to <span class="card__sep"> but NVDA just ignores it.

Could a regular period be used instead of a mid-dot and offset from the baseline to visually appear like a mid-dot? (Personally, even an unstyled period looks fine, but maybe @cherylhjli has thoughts?)

A hacky way to do this would be to add a non-visual span and put a period.

@cherylhjli
Copy link

Could we move the baseline of the period up, so the period resembles a mid-dot?

@greatislander
Copy link
Collaborator Author

@cherylhjli @jhung I just tested with VoiceOver/Safari and realized that VoiceOVer reads 'middle dot' without pausing… so definitely an alternate approach is needed. I'm going to try the period trick.

@greatislander
Copy link
Collaborator Author

Using a period works great in Safari with Voiceover. One observation: this looks weird, but maybe it's okay?

Screen shot of VoiceOver modal which reads: 'language: English . publication date: 2016'.

@jhung
Copy link
Contributor

jhung commented Jan 30, 2020

Using a period works great in Safari with Voiceover. One observation: this looks weird, but maybe it's okay?

Screen shot of VoiceOver modal which reads: 'language: English . publication date: 2016'.

Is there any way to offset the period vertically so it appears in the middle of the line (like a mid-dot)?

@greatislander
Copy link
Collaborator Author

Yes, I can do that with CSS (set vertical-align: super). However that doesn't affect the VoiceOver modal.

@greatislander
Copy link
Collaborator Author

See latest changes from 2120c96.

Copy link
Contributor

@jhung jhung left a comment

Choose a reason for hiding this comment

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

Thanks for fixing the separator. It's much better now.

Cooperative Resource Library 1.0.0 automation moved this from Review in progress to Reviewer approved Jan 30, 2020
Pinecone 1.0.0 automation moved this from Review in progress to Reviewer approved Jan 30, 2020
@greatislander greatislander merged commit cb4e94f into dev Jan 30, 2020
Cooperative Resource Library 1.0.0 automation moved this from Reviewer approved to Done Jan 30, 2020
Pinecone 1.0.0 automation moved this from Reviewer approved to Done Jan 30, 2020
@greatislander greatislander deleted the feat/improve-card-semantics branch January 30, 2020 21:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
3 participants