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

Re-implement and restyle schedule dropdown #2085

Merged
merged 29 commits into from Aug 22, 2018

Conversation

Projects
None yet
4 participants
@hbillings
Member

hbillings commented Aug 8, 2018

Closes #2084. Blocked by #2069.

TODO:

  • Inline dropdown with searchbox and button
  • Remove schedule dropdown from sidebar
  • Style dropdown
  • Reflect currently chosen option as button text
  • Update tests

Note: Moved these to #2084 and #2097 so that we can merge this when it's good to go
- [ ] Add vendor name option to dropdown
- [ ] Hook into vendor search API (relies on #2069)
- [ ] Add contract number option to dropdown
- [ ] Hook into contract number search API (relies on #2069)
- [ ] Replace search radios with "exact match" checkbox

@hbillings hbillings self-assigned this Aug 16, 2018

@hbillings hbillings changed the title from [WIP] Move schedule selector to search bar & add contract/vendor selections to [WIP] Restyle schedule dropdown and add contract/vendor options Aug 16, 2018

@hbillings hbillings changed the title from [WIP] Restyle schedule dropdown and add contract/vendor options to Re-implement and restyle schedule dropdown Aug 16, 2018

@hbillings hbillings requested a review from ericronne Aug 17, 2018

@ericronne

I made a few style changes in a new branch. Other than what i've done in that branch, LGTM!

Merge pull request #2096 from 18F/2084a-search-ui-styling
Style refinements for the 2084 search ui work
@ericronne

Aside from whatever circle ci is grumbling about, all great from where i'm sitting.

@hbillings hbillings requested a review from ericronne Aug 17, 2018

@hbillings hbillings requested a review from tadhg-ohiggins Aug 17, 2018

@hbillings

This comment has been minimized.

Member

hbillings commented Aug 17, 2018

Okay, I think this is ready for technical review (I'm working on fixing the ESLint errors right now).

@@ -582,8 +582,6 @@ class Meta:
@property
def full_name(self):
if self.sin:
return f'{self.sin} - {self.name}'

This comment has been minimized.

@hbillings

hbillings Aug 17, 2018

Member

I'm not sure why we were doing this concatenation in the model, but it made it difficult to have any flexibility with the display of the name.

@@ -1,8 +1,8 @@
import React from 'react';
export function makeOptions(labels) {
export function makeOptions(labels, defaultMsg = '(all)') {

This comment has been minimized.

@hbillings

hbillings Aug 17, 2018

Member

Sometimes we need a different "all options" message.

const setup = makeSetup(SearchCategory, defaultProps, {wrapperOnly: true});
describe('<SearchCategory>', () => {
it('renders correctly', () => {

This comment has been minimized.

@hbillings

hbillings Aug 17, 2018

Member

This is incredibly basic and doesn't test much right now, but since the structure of the HTML is going to change with the addition of the vendor/contract number, it seemed to make sense not to burn too much time on it as of yet.

{/* setting a key event on this div makes it impossible to select
* an option with the keyboard.
* TODO: fix this after contract/vendor name are added and the HTML is in final form. */}
<div /* eslint-disable-line max-len, jsx-a11y/click-events-have-key-events, jsx-a11y/interactive-supports-focus */

This comment has been minimized.

@hbillings

hbillings Aug 17, 2018

Member

This is not ideal, but it works with my keyboard in its current state, so I'm thinking we'll fix it in the vendor/contract PR (which will substantially change the HTML and thus the required ARIA roles anyway).

@abrouilette

This comment has been minimized.

abrouilette commented Aug 20, 2018

"As soon as @tadhg-ohiggins gives the technical review, this is ready to merge."

@ericronne

This comment has been minimized.

Contributor

ericronne commented Aug 21, 2018

Whoops hover color glitch now. Looks like this …
image

but should be …
image
(except for the funky margin fill)

I shall attempt to debug, stay tuned

@hbillings hbillings merged commit ecdbfad into develop Aug 22, 2018

3 checks passed

ci/circleci: build Your tests passed on CircleCI!
Details
security/snyk - package.json (CALC) No manifest changes detected
security/snyk - requirements.txt (CALC) No manifest changes detected

@hbillings hbillings deleted the 2084-search-ui branch Aug 22, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment