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

Migrate rural or underserved tool into cfgov-refresh #5092

Merged
merged 24 commits into from
Jul 24, 2019

Conversation

anselmbradford
Copy link
Member

@anselmbradford anselmbradford commented Jun 27, 2019

Migrates https://github.com/cfpb/rural-or-underserved-test into this repo.

Additions

  • Rural-or-underserved-tool app assets and jinja template.
  • Census API to external API call whitelist.

Testing

  1. Pull branch and run ./frontend.sh && gulp clean && gulp build and run a local server.
  2. Visit http://localhost:8000/rural-or-underserved-tool/ and compare to https://www.consumerfinance.gov/rural-or-underserved-tool/

Screenshots

rural

rural2

Notes

  • There are many classes that can likely be cleaned out, but this provides the initial migration of this app.
  • Layout differs in some respects to the live site by necessity of fitting into a standard layout. Big differences:
    • About section is always visible.
    • Maps in the result table retain a top border line.

Copy link

@accesslint accesslint bot left a comment

Choose a reason for hiding this comment

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

There are accessibility issues in these changes.

<h2>
Check status of properties for loans extended in
<small>
<select id="year" name="year">
Copy link

Choose a reason for hiding this comment

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

Looks like there's a label missing for this input. That makes it hard for people using screen readers or voice control to use the input.

a-text-input__full
input-address"
name="address1"
type="text" />
Copy link

Choose a reason for hiding this comment

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

Looks like there's a label missing for this input. That makes it hard for people using screen readers or voice control to use the input.

Add another address
</a>
</p>
<input class="a-btn" type="submit" value="Check addresses" />
Copy link

Choose a reason for hiding this comment

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

Looks like there's a label missing for this input. That makes it hard for people using screen readers or voice control to use the input.

<input id="file-name"
name="file-name"
class="a-text-input"
value="No file chosen">
Copy link

Choose a reason for hiding this comment

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

Looks like there's a label missing for this input. That makes it hard for people using screen readers or voice control to use the input.

</p>

<p>
<input class="a-btn" type="submit" value="Check addresses" />
Copy link

Choose a reason for hiding this comment

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

Looks like there's a label missing for this input. That makes it hard for people using screen readers or voice control to use the input.

@chosak
Copy link
Member

chosak commented Jun 27, 2019

In Firefox the dropdown has visibly changed from this:

image

to this:

image

Neither of these look great, but the latter looks a bit too squeezed to me, and even more squeezed than the screenshot you shared.

Additionally, the FAQs don't show up as collapsed for me:

image

and in the developer console, I see this warning:

image

This is all running locally, after doing a gulp clean && gulp build as you suggest. I did see this warning during the build:

[12:25:01] Using gulpfile ~/Projects/cfgov-refresh/gulpfile.js
[12:25:01] Starting 'build'...
[12:25:01] Starting 'styles'...
[12:25:01] Starting 'scripts'...
[12:25:01] Starting 'images'...
[12:25:01] Starting 'styles:apps'...
[12:25:01] Starting 'styles:featureFlags'...
[12:25:01] Starting 'styles:ie'...
[12:25:01] Starting 'styles:modern'...
[12:25:01] Starting 'styles:nemo'...
[12:25:01] Starting 'styles:ondemand'...
[12:25:01] Starting 'scripts:polyfill'...
[12:25:01] Starting 'scripts:modern'...
[12:25:01] Starting 'scripts:apps'...
[12:25:01] Starting 'scripts:external'...
[12:25:01] Starting 'scripts:nemo'...
[12:25:01] Starting 'scripts:ondemand'...
[12:25:01] Starting 'images:apps'...
[12:25:01] Starting 'images:general'...
[12:25:01] Starting 'styles:nemoProd'...
[12:25:01] Starting 'styles:nemoIE8'...
[12:25:03] Started generating service worker file...
[12:25:03] Copying eRegs' manifest...
App dependencies not installed, please run from project root: npm --prefix ./cfgov/unprocessed/apps/rural-or-underserved-tool install ./cfgov/unprocessed/apps/rural-or-underserved-tool
[12:25:03] Starting 'scripts:ondemand:header'...
[12:25:03] Starting 'scripts:ondemand:footer'...
[12:25:03] Starting 'scripts:ondemand:nonresponsive'...
[12:25:03] Finished 'styles:featureFlags' after 1.91 s
[12:25:03] webpack-stream - No files given; aborting compilation
[12:25:03] Successfully copied eRegs' manifest to ./cfgov/static_built/apps/regulations3k/regulations3k-manifest.json
[12:25:03] Generated cfgov/regulations3k/jinja2/regulations3k/regulations3k-service-worker.js, which will precache 0 files, totaling 0 bytes.

Note the message about ROUT dependencies not being installed. Is this expected?

Copy link

@accesslint accesslint bot left a comment

Choose a reason for hiding this comment

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

There are accessibility issues in these changes.

cfgov/jinja2/v1/rural-or-underserved/index.html Outdated Show resolved Hide resolved
cfgov/jinja2/v1/rural-or-underserved/index.html Outdated Show resolved Hide resolved
@anselmbradford
Copy link
Member Author

Updated drop-down to use CF a-select styling in 84a379e

Screen Shot 2019-06-27 at 1 11 25 PM

It's pretty tight at mobile. Not sure how we want to handle that.

Copy link

@accesslint accesslint bot left a comment

Choose a reason for hiding this comment

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

👏 You fixed the issue(s)! Great work.

@niqjohnson
Copy link
Member

@anselmbradford: I pushed up a change to convert the FAQs from individual expandables to expandable groups to match a little more closely what's there now. Take a look at that and see what you think.

Also, there's a slight style bug I'm seeing where the expandable headings aren't vertically centered because they're still getting the margin-bottom from .h4 applied to them:

Current Expected
current expected

Looking at other expandable groups (like the ones on the TILA-RESPA Integrated Disclosure FAQs page), I think this might have something to do with the order the Capital Framework styles are getting imported? It looks like .o-expandable_label is meant to override the bottom margin on .h4, but instead .h4 is overriding .o-expandable_label. It's a minor thing, but I wasn't sure the best way to fix it.

- Float the show/hide map button to the right
- Hide the border between result and map
- Reduce the top and bottom margins around result count
- Bump up the font size of the result counters
@niqjohnson
Copy link
Member

@anselmbradford, I pushed up a couple more small style updates (more info on those in the GHE issue). I'm pretty secure in the design side of the updates but much less secure about the CSS I used to make the design side happen, so feel free to give the code a look and change anything that needs changing.

@niqjohnson
Copy link
Member

@anselmbradford, one other thing I noticed while I was poking around: it looks like the body of the page is getting an overflow-x: hidden; style from somewhere at small screen sizes (maybe mega-menu.less if Chrome's dev tools are to be believed). That's preventing the page from horizontally scrolling on small screens to let you view the full width of the result tables.

@anselmbradford
Copy link
Member Author

anselmbradford commented Jul 11, 2019

one other thing I noticed while I was poking around: it looks like the body of the page is getting an overflow-x: hidden; style from somewhere at small screen sizes (maybe mega-menu.less if Chrome's dev tools are to be believed). That's preventing the page from horizontally scrolling on small screens to let you view the full width of the result tables.

Hmm, yeah most definitely from the mega-menu, since the hamburger menu sits alongside the page's contents to enable it to slide, so this sounds like an issue we need to resolve within the table itself.

@niqjohnson
Copy link
Member

@anselmbradford: I wonder if we should convert the tables to the horizontal scroll version? Seems like in theory what we want to happen, but I don't think I've every actually seen one of our scrolling tables in the wild yet, so I'm not sure if that's the right way to go.

@anselmbradford
Copy link
Member Author

Also, there's a slight style bug I'm seeing where the expandable headings aren't vertically centered because they're still getting the margin-bottom from .h4 applied to them:

@niqjohnson I fixed this in 1363d77 You were right, the order was wrong (actually cf-core was being delivered twice).

@anselmbradford
Copy link
Member Author

I wonder if we should convert the tables to the horizontal scroll version? Seems like in theory what we want to happen, but I don't think I've every actually seen one of our scrolling tables in the wild yet, so I'm not sure if that's the right way to go.

Converted to scrolling tables in e9a8167
Also, I converted the summary counters at the top (the large numbers) to inline-flex instead of inline-grid, since they overlapped with the text if they were two digits.
Lastly, I wasn't sure if the show more buttons were too close?

Screen Shot 2019-07-12 at 8 03 09 PM

Copy link
Member

@niqjohnson niqjohnson left a comment

Choose a reason for hiding this comment

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

This is looking good. I just had a few comments: two that seem to come out of our typical scrollable table styles and one on adding a little more space between the buttons.

cfgov/jinja2/v1/rural-or-underserved/index.html Outdated Show resolved Hide resolved

<p class="block block__flush-top block__flush-bottom block__padded-top block__padded-bottom">
<a data-table="rural" class="button-more u-hidden a-btn a-btn__secondary no-decoration" href="#" id="ruralMore">View 10 more</a>
<a data-table="rural" class="margin-left-1 view-all u-hidden no-visit" href="#" id="ruralAll">View all</a>
Copy link
Member

Choose a reason for hiding this comment

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

Let's give these buttons the same separation as similar button combos on cf.gov, like the "Apply filters" and "Clear filters" buttons in filter modules. I think that's a 5px separation.

Copy link
Member Author

Choose a reason for hiding this comment

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

It'll be updated to this in the next push
Screen Shot 2019-07-16 at 10 18 24 AM

Copy link
Member Author

Choose a reason for hiding this comment

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

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 all be button elements, too.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, true

Copy link
Member

Choose a reason for hiding this comment

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

The spacing looks good to me

@anselmbradford anselmbradford merged commit 3df2470 into master Jul 24, 2019
@anselmbradford anselmbradford deleted the migrate_rural branch July 24, 2019 14:03
@chosak chosak mentioned this pull request Jul 25, 2019
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.

5 participants