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 collections to publisher metadata if B variant #1049

Merged
merged 1 commit into from
Aug 15, 2018

Conversation

andrewgarner
Copy link
Contributor

@andrewgarner andrewgarner commented Aug 15, 2018

@andrewgarner andrewgarner requested a review from sihugh August 15, 2018 10:53
@benthorner benthorner temporarily deployed to government-frontend-pr-1049 August 15, 2018 10:53 Inactive
@vanitabarrett
Copy link
Contributor

This might be a design question: do we want "and 2 others" as well as "+ show all"? I think previously we've combined the two, so "+ 2 others" (for example) becomes the toggle link.

@andrewgarner
Copy link
Contributor Author

andrewgarner commented Aug 15, 2018

When I looked at this with Alan for the top banner we had a couple of assumption that led to this approach:

  1. the "+" or "-" suggests that the link will show or hide and not take you to another page which may deter interaction.
  2. "and X others" for a hyperlink that hides an element on the page doesn't describe the behaviour.

<a href="#"
data-controls="<%= toggle_id %>"
data-expanded="false"
data-toggled-text="&minus; hide all">+ show all</a>
Copy link
Contributor

Choose a reason for hiding this comment

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

These should probably be translated strings

Copy link
Contributor Author

@andrewgarner andrewgarner Aug 15, 2018

Choose a reason for hiding this comment

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

@vanitabarrett I've added translation support with 3a01e37 but we don't have actual translations.

@benthorner benthorner temporarily deployed to government-frontend-pr-1049 August 15, 2018 11:48 Inactive
@andrewgarner andrewgarner force-pushed the add-collections-to-publisher-metadata branch from 4897126 to 3a01e37 Compare August 15, 2018 11:52
@benthorner benthorner temporarily deployed to government-frontend-pr-1049 August 15, 2018 11:52 Inactive
@benthorner benthorner temporarily deployed to government-frontend-pr-1049 August 15, 2018 11:59 Inactive
@andrewgarner andrewgarner force-pushed the add-collections-to-publisher-metadata branch from d2e9c59 to 2fb963f Compare August 15, 2018 13:05
@benthorner benthorner temporarily deployed to government-frontend-pr-1049 August 15, 2018 13:05 Inactive
@vanitabarrett
Copy link
Contributor

@andrewgarner As this is a change to the publisher metadata component, we probably need to add a test in publisher_metadata_spec.rband an example in the component yml docs (as adding a collection has specific behaviour, especially when there are more than 2)

@andrewgarner andrewgarner force-pushed the add-collections-to-publisher-metadata branch from 2fb963f to 09dde92 Compare August 15, 2018 14:21
@benthorner benthorner temporarily deployed to government-frontend-pr-1049 August 15, 2018 14:21 Inactive
@andrewgarner andrewgarner force-pushed the add-collections-to-publisher-metadata branch from 09dde92 to 4e4b894 Compare August 15, 2018 14:34
@andrewgarner andrewgarner merged commit a6c799d into master Aug 15, 2018
@andrewgarner andrewgarner deleted the add-collections-to-publisher-metadata branch August 15, 2018 14:46
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