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

Wrap document capture error message at bounds of file input #4324

Merged
merged 9 commits into from Oct 22, 2020

Conversation

aduth
Copy link
Member

@aduth aduth commented Oct 21, 2020

Why: Per design feedback.

Before After
before after

Implementation Notes:

This turned out to require a messy cascade of unrelated revisions, triggered by a desire to adopt CSS imported via React components (introduced in #4260). I've made a point to describe the purpose of each of these changes in detailed commit history. Ultimately, it's made worse by outdated and implicit dependencies (see also 18F/identity-design-system#159).

I think these are worth working through in order to benefit from advantages of granular CSS bundles, but for this purpose of this requirement, it might be best to either...

  • Limit to contain changes to the main application CSS bundle (app/assets/stylesheets/components/_file-input.scss)
  • Or: Split out some of the unrelated changes to separate pull requests
  • Or: Wait for LG-3187: Upgrade USWDS to 2.9.0 identity-design-system#159, which will simplify some of the more unfortunate concessions

Copy link
Contributor

@zachmargolis zachmargolis left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@anniehirshman-gsa anniehirshman-gsa left a comment

Choose a reason for hiding this comment

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

Visually, screenshot LGTM!

Note: I updated the text in the error inventory, which will avoid the orphan for this specific message in the screenshot.

**Why**: AirBnb base allow-lists an expected set of patterns for development, not including postcss.config.js. This configuration is both inextensible and excessively restrictive. The default should suffice for the essence of the rule.

See: https://github.com/airbnb/javascript/blob/ee2f22a10c0b82a21eaa7743905712b6cb145fc2/packages/eslint-config-airbnb-base/rules/imports.js#L72-L94
**Why**: USWDS includes many comments in its output, which are not removed by sass-loader, even in either of "compressed" and "compact" output styles.
**Why**: Since these bundles are compiled separately from application SCSS, if bundled SCSS relies on design system functions or variables, they must be preloaded. Preloading at loader configuration avoids need to include in each bundle.
**Why**: Ensure error message wraps at bounds of the file input
@aduth
Copy link
Member Author

aduth commented Oct 22, 2020

Note: I updated the text in the error inventory, which will avoid the orphan for this specific message in the screenshot.

For context, the updated messaging:

This file type is not accepted, please choose a JPG, PNG, HEIC, BMP, or TIFF file.

Annie and I chatted briefly about this, since part of my intention with these changes was to refactor the component slightly to allow it to be more reusable. A message like this is challenging, since not all usages of a file input component would need these specific file types.

What we landed on was to allow the text to be customized where it's used, just like with other texts (hint, etc), so that we can still tailor the message appropriately to the hint.

I've updated this in 669ff6d and 32cadc2. Here's how the customized message looks:

image

Since we'd still want a fallback text in case a customized message is not provided in other usage, I've left this from the initial implementation. Though based on @anniehirshman-gsa 's feedback, shortened it slightly in 06cff29 to allow it to fit on one line:

image

Copy link
Contributor

@anniehirshman-gsa anniehirshman-gsa left a comment

Choose a reason for hiding this comment

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

Shortened for consistency with other error messages. Thanks!

config/locales/errors/en.yml Outdated Show resolved Hide resolved
config/locales/errors/es.yml Outdated Show resolved Hide resolved
config/locales/errors/fr.yml Outdated Show resolved Hide resolved
Co-authored-by: Annie Hirshman (she/her) <66273797+anniehirshman-gsa@users.noreply.github.com>
@aduth
Copy link
Member Author

aduth commented Oct 22, 2020

Thanks for catching that mistake @anniehirshman-gsa . Updated in 28ea67e.

Copy link
Contributor

@anniehirshman-gsa anniehirshman-gsa left a comment

Choose a reason for hiding this comment

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

LGTM

@aduth aduth merged commit 11f7bd5 into master Oct 22, 2020
@aduth aduth deleted the aduth-error-wrap branch October 22, 2020 18:34
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.

None yet

3 participants