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

fix: call-to-action, accordion, tags, and filter list state styling #380

Merged
merged 13 commits into from
Jun 25, 2020

Conversation

jhung
Copy link
Contributor

@jhung jhung commented Jun 3, 2020

Description

This PR addresses a number of styling issues:

Steps to test

  1. Go to the deployed instance for this PR: https://deploy-preview-380--pinecone.netlify.app/
  2. Examine:
    • Atom > Tag
    • Molecule > Accordion
    • Molecule > Filter List
    • Layout > Search Results (check Filter list)
    • Layout > Archive (check Filter list)
    • Layout > Resource (check Tags)

Expected behavior:

The focus, hover, and active states should look like the design spec in major current browsers.

Related issues

Fixes #359
Fixes #366
Fixes #362
Fixes #316

@jhung jhung added this to the 1.0.0-beta.3 milestone Jun 3, 2020
@jhung jhung self-assigned this Jun 3, 2020
@netlify
Copy link

netlify bot commented Jun 3, 2020

Deploy preview for pinecone ready!

Built with commit d2374c8

https://deploy-preview-380--pinecone.netlify.app

@jhung jhung marked this pull request as ready for review June 8, 2020 14:11
@jhung jhung requested review from greatislander and cherylhjli June 8, 2020 14:11
@jhung jhung added bug Something isn't working enhancement New feature or request labels Jun 8, 2020
@jhung jhung changed the title fix: call-to-action now has inverted styles fix: call-to-action, accordion, tags, and filter list state styling Jun 8, 2020
Copy link
Collaborator

@greatislander greatislander left a comment

Choose a reason for hiding this comment

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

There's a weird accordion behaviour in Firefox when a button is active and focussed:

Screen Shot 2020-06-08 at 8 16 05 AM

The border on the bottom should be hidden if possible.

@jhung
Copy link
Contributor Author

jhung commented Jun 8, 2020

@greatislander the issue you observed only happens if you use the debugger. If using keyboard or mouse to activate the various states, the top-border of the item below properly hides.

Putting PR back into draft - I found an issue with the mobile view on Filter List that needs to be fixed.

@greatislander
Copy link
Collaborator

@jhung It happens for me during regular use too.

@greatislander
Copy link
Collaborator

@jhung Will you have any capacity to wrap this up on Monday? Would like to get these styles into a release.

@cherylhjli
Copy link

hey @jhung, a few things come up for me -

  • In the filter list for the archive layout and search result layout, the filter list checkboxes disappear or are a very low contrast colour in all browsers:

Screen Shot 2020-06-22 at 11 02 41 AM
Screen Shot 2020-06-22 at 11 02 51 AM
Screen Shot 2020-06-22 at 11 01 51 AM
Screen Shot 2020-06-22 at 11 02 36 AM

Also noticed that in safari, in the resource page, the CTA text is black instead of white:
Screen Shot 2020-06-22 at 11 05 04 AM

@greatislander
Copy link
Collaborator

@jhung The filter list form needs a .form class (that should resolve the first of @cherylhjli's comments). As for the CTA I'm not sure but that needs to be resolved as this PR is supposed to fix CTAs.

@jhung
Copy link
Contributor Author

jhung commented Jun 23, 2020

@cherylhjli - fixed the CTA issue and the checkbox borders not appearing correctly.

There's another issue with the Archive filter list - all the child filter items are all showing up as "checkbox". This will be looked at in a different PR.

@cherylhjli
Copy link

@jhung the CTA looks great, the checkboxes are still disappearing for me?

@greatislander greatislander self-requested a review June 23, 2020 19:53
Copy link

@cherylhjli cherylhjli left a comment

Choose a reason for hiding this comment

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

This looks great, Jon. Checkboxes are good. Tiny thing - I noticed that the checkboxes in subcategories are not selectable if you click the label, whereas the checkboxes that are on the top level are.

@greatislander greatislander self-requested a review June 23, 2020 20:54
Copy link
Collaborator

@greatislander greatislander left a comment

Choose a reason for hiding this comment

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

I missed that issue @cherylhjli identified; I think it should be resolved in this PR. Sorry @jhung. I expect it is related to the "Checkbox" issue.

@jhung
Copy link
Contributor Author

jhung commented Jun 24, 2020

I missed that issue @cherylhjli identified; I think it should be resolved in this PR. Sorry @jhung. I expect it is related to the "Checkbox" issue.

The IDs were being all set to 'checkbox', so it appeared that the checkboxes were not working. The logic for setting the for and id in the checkbox template doesn't seem to be working if a value was not set in the template configuration file.

For now I've worked around the issue by giving values for all children checkboxes. Ideally the ID generation for the checkbox should be less fragile (perhaps using faker or nunjucks random to generate unique IDs where needed).

@greatislander
Copy link
Collaborator

@jhung jhung#1

@greatislander greatislander self-requested a review June 24, 2020 16:39
@greatislander greatislander merged commit 08b017a into platform-coop-toolkit:dev Jun 25, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working enhancement New feature or request
Projects
None yet
3 participants