-
Notifications
You must be signed in to change notification settings - Fork 166
React Image Integration #3911
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
React Image Integration #3911
Changes from all commits
eda7473
a15bafc
5bd4dd6
7b7e63c
4077107
3e8d093
4995486
18e8fc6
fd4df7f
cabd551
dcd4e5c
4e38c48
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,2 +1,3 @@ | ||
| //= require i18n-strings | ||
| //= require local-time | ||
| //= require asset-strings |
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,12 @@ | ||||||||||||||||||||||||||||||||||||||||||||||
| 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) %>'; | ||||||||||||||||||||||||||||||||||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As used in |
||||||||||||||||||||||||||||||||||||||||||||||
| <% end %> | ||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+1
to
+12
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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:
Suggested change
In my testing, this creates an object mapping the original path to the one including the digest: |
||||||||||||||||||||||||||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,5 +1,11 @@ | ||
| import React from 'react'; | ||
| import useI18n from '../hooks/use-i18n'; | ||
| import { useImage } from '../hooks/use-assets'; | ||
|
|
||
| function DocumentCapture() { | ||
| return 'Document Capture'; | ||
| const t = useI18n(); | ||
| const imageTag = useImage(); | ||
| return <img src={imageTag('idv/phone.png')} alt={t('doc_auth.headings.welcome')} />; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If the idea is to have this correspond to an equivalent Rails helper method, and since
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 const getAssetPath = useAssets();
const src = getAssetPath('images/idv/phone.png'); |
||
| } | ||
|
|
||
| export default DocumentCapture; | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| import { createContext } from 'react'; | ||
|
|
||
| const AssetContext = createContext({}); | ||
|
|
||
| export default AssetContext; |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| import { createContext } from 'react'; | ||
|
|
||
| const I18nContext = createContext({}); | ||
|
|
||
| export default I18nContext; |
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -0,0 +1,14 @@ | ||||||
| import { useContext } from 'react'; | ||||||
| import AssetContext from '../context/assets'; | ||||||
|
|
||||||
| function useImage() { | ||||||
| const strings = useContext(AssetContext); | ||||||
| const imageStrings = strings.images || {}; | ||||||
| const imageTag = (key) => { | ||||||
| const resolvedImage = imageStrings[key]; | ||||||
| return resolvedImage !== undefined ? resolvedImage : key; | ||||||
| }; | ||||||
| return imageTag; | ||||||
| } | ||||||
|
|
||||||
| export { useImage }; | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 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 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 |
||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,10 @@ | ||
| import { useContext } from 'react'; | ||
| import I18nContext from '../context/i18n'; | ||
|
|
||
| function useI18n() { | ||
| const strings = useContext(I18nContext); | ||
| const t = (key) => (Object.prototype.hasOwnProperty.call(strings, key) ? strings[key] : key); // eslint-disable-line | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What was the reason for disabling ESLint for this line? |
||
| return t; | ||
| } | ||
|
|
||
| export default useI18n; | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,10 +1,18 @@ | ||
| import React from 'react'; | ||
| import { render } from 'react-dom'; | ||
| import DocumentCapture from '../app/document-capture/components/document-capture'; | ||
| import AssetContext from '../app/document-capture/context/assets'; | ||
| import I18nContext from '../app/document-capture/context/i18n'; | ||
|
|
||
| const { I18n: i18n, AssetStrings } = window.LoginGov; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This relates to a review comment at #3909 (review) which I'll need to respond to, but the idea with renaming 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 |
||
|
|
||
| const appRoot = document.getElementById('document-capture-form'); | ||
| appRoot.innerHTML = ''; | ||
| render( | ||
| <DocumentCapture />, | ||
| <AssetContext.Provider value={AssetStrings}> | ||
| <I18nContext.Provider value={i18n.strings[i18n.currentLocale()]}> | ||
| <DocumentCapture /> | ||
| </I18nContext.Provider> | ||
| </AssetContext.Provider>, | ||
| appRoot, | ||
| ); | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,16 @@ | ||
| import React, { useContext } from 'react'; | ||
| import { render } from '@testing-library/react'; | ||
| import I18nContext from '../../../../../app/javascript/app/document-capture/context/i18n'; | ||
| import { useDOM } from '../../../support/dom'; | ||
|
|
||
| describe('document-capture/context/i18n', () => { | ||
| useDOM(); | ||
|
|
||
| const ContextValue = () => JSON.stringify(useContext(I18nContext)); | ||
|
|
||
| it('defaults to empty object', () => { | ||
| const { container } = render(<ContextValue />); | ||
|
|
||
| expect(container.textContent).to.equal('{}'); | ||
| }); | ||
| }); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,27 @@ | ||
| import React from 'react'; | ||
| import { render } from '@testing-library/react'; | ||
| import I18nContext from '../../../../../app/javascript/app/document-capture/context/i18n'; | ||
| import useI18n from '../../../../../app/javascript/app/document-capture/hooks/use-i18n'; | ||
| import { useDOM } from '../../../support/dom'; | ||
|
|
||
| describe('document-capture/hooks/use-i18n', () => { | ||
| useDOM(); | ||
|
|
||
| const LocalizedString = ({ stringKey }) => useI18n()(stringKey); | ||
|
|
||
| it('returns localized key value', () => { | ||
| const { container } = render( | ||
| <I18nContext.Provider value={{ sample: 'translation' }}> | ||
| <LocalizedString stringKey="sample" /> | ||
| </I18nContext.Provider>, | ||
| ); | ||
|
|
||
| expect(container.textContent).to.equal('translation'); | ||
| }); | ||
|
|
||
| it('falls back to key value', () => { | ||
| const { container } = render(<LocalizedString stringKey="sample" />); | ||
|
|
||
| expect(container.textContent).to.equal('sample'); | ||
| }); | ||
| }); |

There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did it turn out that this is necessary?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I believe it is