DAH-1360: Upgrade Partners to React 18 #584
Conversation
chadbrokaw
force-pushed
the
DAH-1360_React_18_upgrade
branch
from
e63d4f1
to
05a0a3d
Compare
chadbrokaw
changed the title
.vscode/settings.json
Outdated
| "jest.disabledWorkspaceFolders": ["spec/javascript/e2e"], | ||
| "editor.codeActionsOnSave": { | ||
| // For ESLint |
There was a problem hiding this comment.
suggestion: comments are not officially supported in JSON, and this comment seems unnecessary.
| @@ -100,11 +100,11 @@ gem "slim-rails", "~> 3.1" | |||
| # Added at 2017-10-25 00:30:33 -0700 by dkaplan: | |||
| gem "hashie" | |||
|
|
|||
| # Added at 2017-10-29 23:29:10 -0700 by dkaplan: | |||
There was a problem hiding this comment.
thought: are these comments even necessary? We have git to record this information
There was a problem hiding this comment.
Yeah, they seem like clutter to me... I'm good to remove them
| const prefName = selectedPreference.lottery_preference.name | ||
| form.change(`preferences[${i}].recordtype_developername`, recordTypeMap[prefName] || 'Custom') | ||
| } | ||
| React.useEffect(() => { |
There was a problem hiding this comment.
thought: I've noticed that we use custom versions of useEffect and useState hooks, which are defined in customHooks.js. We might want to revisit this later to determine if we actually need the custom versions. Maybe at the time they were implemented, React hooks were not mature enough to meet our needs?
There was a problem hiding this comment.
Good catch. There are likely some of those that are not needed. I created a ticket to investigate this further: https://sfgovdt.jira.com/browse/DAH-1943
|
|
||
| React.useEffect(() => { |
There was a problem hiding this comment.
chore: can you include a section on how to if google analytics is working, in the Review Instructions?
There was a problem hiding this comment.
Sure! I added a section for how to test as well as an image that proves that the system is correctly talking with GA
| }) | ||
|
|
||
| test('should render the checkbox with indeterminate=true', () => { | ||
| expect(wrapper.find(Checkbox).props().indeterminate).toBeTruthy() | ||
| // Even though the checkbox is in the indeterminate state, it is still considered checked |
There was a problem hiding this comment.
suggestion: make the assertion cleaner with toBePartiallyChecked
There was a problem hiding this comment.
Cool! Love that
* fix: app works, working through tests * More tests * Atom tests * next wave of tests * more tests * add mini-css-extract * Fix rspec tests * Add more time to failing test * fix dep * fix dep * fix webpack * postcss-preset-env * clean up testing * Fix test snapshot * cleanup * JL PR Feedback * e2e issues
DAH-1360
This PR touches virtually everything in the application. There is nothing specific to test except ensuring that everything looks okay and that the communication between the front and back end is working as expected. A few things this PR does: