Conversation
**Why**: As a login.gov developer, I want to be able to test React functionality, and for those tests to be integrated with the existing Mocha test framework.
**Why**: As a user, I want pages that I'm viewing to be localized to my language, including those page which use React.
Leftover from earlier iterations
| } | ||
|
|
||
| <% image_keys.each do |key| %> | ||
| window.LoginGov.AssetStrings.images['<%=key %>'] = '<%= ActionController::Base.helpers.image_path(key) %>'; |
There was a problem hiding this comment.
As used in i18n-strings.js.erb, the j helper is an alias for escape_javascript, which may be important to help escape potentially unexpected / conflicting characters (e.g. early terminating single quote ' or </script>, etc).
| return 'Document Capture'; | ||
| const t = useI18n(); | ||
| const imageTag = useImage(); | ||
| return <img src={imageTag('idv/phone.png')} alt={t('doc_auth.headings.welcome')} />; |
There was a problem hiding this comment.
If the idea is to have this correspond to an equivalent Rails helper method, and since image_tag in Rails would generate the full markup for the image, should we consider to refer to this as imagePath, since it's intended to correspond to image_path (return the src value)?
There was a problem hiding this comment.
I also wonder if, rather than making separate helpers for each type of asset, it would be possible to create one generic helper which can return the path for any asset, regardless of type.
Considering asset_path as an alternative to image_path, where the usage could be something like:
const getAssetPath = useAssets();
const src = getAssetPath('images/idv/phone.png');|
|
||
| function useI18n() { | ||
| const strings = useContext(I18nContext); | ||
| const t = (key) => (Object.prototype.hasOwnProperty.call(strings, key) ? strings[key] : key); // eslint-disable-line |
There was a problem hiding this comment.
What was the reason for disabling ESLint for this line?
| import AssetContext from '../app/document-capture/context/assets'; | ||
| import I18nContext from '../app/document-capture/context/i18n'; | ||
|
|
||
| const { I18n: i18n, AssetStrings } = window.LoginGov; |
There was a problem hiding this comment.
This relates to a review comment at #3909 (review) which I'll need to respond to, but the idea with renaming LoginGov.I18n to i18n was to adhere to camel-case conventions with lowercase first letter. The same could apply to AssetStrings.
But it depends on how we'd want to approach the naming convention, since there is some precedent in the broader ecosystem for naming fixed enumerated sets of values in this way as PascalCase (for example, see Google style guide or TypeScript enum documentation).
| //= link application.css | ||
|
|
||
| //= link i18n-strings.js | ||
| //= link asset-strings.js |
There was a problem hiding this comment.
Did it turn out that this is necessary?
There was a problem hiding this comment.
Yes, I believe it is
Co-authored-by: Andrew Duthie <andrew.duthie@gsa.gov>
| window.LoginGov = window.LoginGov || {} | ||
| <% image_keys = [ | ||
| 'idv/phone.png' | ||
| ] %> | ||
|
|
||
| window.LoginGov.AssetStrings = { | ||
| images: {} | ||
| }; | ||
|
|
||
| <% image_keys.each do |key| %> | ||
| window.LoginGov.AssetStrings.images['<%=key %>'] = '<%= ActionController::Base.helpers.image_path(key) %>'; | ||
| <% end %> |
There was a problem hiding this comment.
Based on our previous conversation, I think we should be able to make this less opinionated about the particular kinds of assets, instead creating a single object which can contain any of the paths we might want in the client:
| window.LoginGov = window.LoginGov || {} | |
| <% image_keys = [ | |
| 'idv/phone.png' | |
| ] %> | |
| window.LoginGov.AssetStrings = { | |
| images: {} | |
| }; | |
| <% image_keys.each do |key| %> | |
| window.LoginGov.AssetStrings.images['<%=key %>'] = '<%= ActionController::Base.helpers.image_path(key) %>'; | |
| <% end %> | |
| window.LoginGov = window.LoginGov || {}; | |
| window.LoginGov.AssetStrings = {}; | |
| <% keys = [ | |
| 'idv/phone.png' | |
| ] %> | |
| <% keys.each do |key| %> | |
| window.LoginGov.AssetStrings['<%= ActionController::Base.helpers.j key %>'] = '<%= ActionController::Base.helpers.j ActionController::Base.helpers.asset_path key %>'; | |
| <% end %> |
In my testing, this creates an object mapping the original path to the one including the digest:
| return imageTag; | ||
| } | ||
|
|
||
| export { useImage }; |
There was a problem hiding this comment.
In separate work I came across the need to recreate an existing shared view element in React (the accordion), and it occurred to me that it would be a nice abstraction to be able to use an image as a regular <img> HTML tag, in a way that the application would substitute the digest-modified path automatically. I was considering this a new Image component.
Example usage:
<Image
src="plus.svg"
alt={ t('image_description.accordian_plus_buttom') }
width={ 16 }
className="plus-icon display-none"
/>Recreating this:
identity-idp/app/views/shared/_accordion.html.slim
Lines 17 to 18 in 982dc4a
Repurposing src as a reference to an asset path might be a bit too magical, so naming it something like assetPath could be more appropriate.
As far as it relates here, I still think we'll need the asset context, but I'm not yet sure about these hooks. It seems like if we had an Image component abstraction, it wouldn't be too much a regular bother to reference the context using useContext, without any intermediary useAsset or useImage hooks.
|
Duplicated by #3931 |
2 participants
No description provided.