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

[MDS-6129] Clean-up in project summary and add react-redux typing #3293

Merged
merged 3 commits into from
Nov 6, 2024

Conversation

taraepp
Copy link
Collaborator

@taraepp taraepp commented Nov 5, 2024

Objective

  • making a PR before making the real PR with some work that might be out of scope...
  • I like library imports at the top, before system imports. 😁
  • some linter issues, like React Fragments being used where they don't do anything
  • there's also a loop that doesn't seem to be using its loopiness but I didn't want to try breaking it just yet. I let the linter turn it from a let i loop to a for const loop though.
  • that <a> I took out wasn't linking anywhere
  • I didn't like that the isFieldDisabled function is specific to projects but was in a generic utils file with a generic-sounding name (probably put in because removeNullValuesRecursive is there). But I also don't like that it's not specific to the field it's potentially disabling and therefore doesn't need to be called 88 times. Still deciding whether to just call it once (or max once per file) or turn it into a createSelector or memoize it.
  • added react-redux typing
    • you can kind of tell by where the @ts-ignores are added is where our current typing isn't working out
    • technically, it wasn't working out before, but now it's telling us about it!
    • my compiler shows 142 type errors now (from all over), and I'm pretty sure they weren't there before. 🤦‍♀️
    • it doesn't like the dispatch(args).then(func)
    • it also doesn't like the state being passed to userHasRole and similar stuff
    • I briefly experimented with making FormWrapper a generic by copying the approach that antd uses (kind of neat! <Values = any>- so it's generic but if you don't give it a type it defaults to any) but it wasn't working and it was way too complicated with redux-form, which initializes FormData as {}, which really interferes with typing (hence the as IProjectSummaryForm instead of formValues: IProjectSummaryForm)
  • I'm not dead-set on the way I set up those interfaces for IProjectSummaryForm. I just quickly added attributes that seemed new or different into a new interface.
  • Also not dead-set on the name IProjectSummaryForm, but especially due to the changes in authorizations, it seemed better to separate out as 2 different interfaces.

MDS-6129

Why are you making this change? Provide a short explanation and/or screenshots

Copy link

sonarqubecloud bot commented Nov 6, 2024

Quality Gate Passed Quality Gate passed for 'bcgov-sonarcloud_mds_minespace-web'

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

Copy link

sonarqubecloud bot commented Nov 6, 2024

Quality Gate Failed Quality Gate failed for 'bcgov-sonarcloud_mds_common'

Failed conditions
79.8% Coverage on New Code (required ≥ 80%)

See analysis details on SonarCloud

Copy link

sonarqubecloud bot commented Nov 6, 2024

Quality Gate Passed Quality Gate passed for 'bcgov-sonarcloud_mds_core-web'

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

Copy link
Collaborator

@simensma-fresh simensma-fresh left a comment

Choose a reason for hiding this comment

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

Yay for more typings! For the future we should deal with those ts-ignores, think we're just missing an unwrap() call. But that's probably best dealt with on a case by case basis

https://redux-toolkit.js.org/api/createAsyncThunk#unwrapping-result-actions

@taraepp taraepp merged commit d2c6732 into develop Nov 6, 2024
13 of 14 checks passed
@taraepp taraepp deleted the mds-6129-project-summary-status branch November 6, 2024 22:22
@taraepp taraepp restored the mds-6129-project-summary-status branch November 6, 2024 22:22
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.

3 participants