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.

Sign up for GitHub

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

Jump to bottom

DF-1964-contact-filters #589

Merged
merged 4 commits into from
May 27, 2015
Merged

DF-1964-contact-filters #589

merged 4 commits into from
May 27, 2015

Conversation

jimmynotjim
Copy link
Contributor

Updated contact us filter to use new notifications

Additions

None

Removals

  • Removes "show all" button

Changes

  • Replaces type-and-filter messaging with cf_notifier

Testing

  • Checkout branch and use the contact filters at /contact-us/

Review

Preview

screen shot 2015-05-26 at 1 47 28 pm

screen shot 2015-05-26 at 1 47 40 pm

screen shot 2015-05-26 at 1 57 07 pm

Preview this PR without the whitespace changes

Notes

None

James Wilson added 2 commits May 26, 2015 13:36
Updated contact us filter to use new notifications
574f184 
- Replaces type-and-filter messaging with cf_notifier
- Removes "show all" button
Updating for correct two line notifications
5b78921
@jimmynotjim jimmynotjim mentioned this pull request May 26, 2015
Merged
sebworks
sebworks reviewed May 27, 2015
View reviewed changes
src/static/js/modules/jquery/type-and-filter.js
state = 'warning';
}

message = message.replace( /{{[\s]*term[\s]*}}/, searchTerm );
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it possible to combine into one regex and use matching parens like found here https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/RegExp ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the existing code, just moved around. I was actually thinking about replacing it with handlebars since we're already including it as a dependency. Just seemed out of scope for this PR.

@sebworks
Copy link
Contributor

👍

@KimberlyMunoz
Copy link
Contributor

👍

On Wed, May 27, 2015 at 1:55 PM sebworks [email protected] wrote:

[image: 👍]

—
Reply to this email directly or view it on GitHub
#589 (comment).

@anselmbradford
Copy link
Member

👍 Tested locally and works. Two thoughts that don't have to happen in this PR:

  • The animation appears way too slow to me. Just saw a presentation by CSS animation guru Val Head last week and she said to build animations and then double the speed when you're happy with the motion because you get used to it being slow. 0.2 to 0.5 second for the transition is the normal range to shoot for.
  • The filters are a great candidate for building out our test suite further as there are lots of interactions and combinations that are time consuming to manually test.

@jimmynotjim
Copy link
Contributor Author

@anselmbradford Experimenting with some other options for speed. Will open a new PR to discuss.

@jimmynotjim jimmynotjim force-pushed the DF-1964-contact-filters branch from 7603f08 to 8562494 Compare May 27, 2015 20:18
Updated to remove default status notification
f81dee9 
- No longer shows the default status on page load or after clearing the input.
- Separates out the button/input listeners so either or both can be used
- Removes duplicate seach submissions
@jimmynotjim jimmynotjim force-pushed the DF-1964-contact-filters branch from 8562494 to f81dee9 Compare May 27, 2015 20:20
jimmynotjim added a commit that referenced this pull request May 27, 2015
@jimmynotjim
Merge pull request #589 from cfpb/DF-1964-contact-filters
35b079b 
DF-1964-contact-filters
@jimmynotjim jimmynotjim merged commit 35b079b into flapjack May 27, 2015
@jimmynotjim jimmynotjim deleted the DF-1964-contact-filters branch May 27, 2015 20:20
@jimmynotjim jimmynotjim mentioned this pull request May 27, 2015
Merged
@jimmynotjim jimmynotjim mentioned this pull request Aug 12, 2015
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@jimmynotjim @sebworks @KimberlyMunoz @anselmbradford @Scotchester