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

✨ Jobfunctions: Update Notification and refactors update/create modal and queries #1209

Merged
merged 3 commits into from
Jul 27, 2023
Merged

✨ Jobfunctions: Update Notification and refactors update/create modal and queries #1209

merged 3 commits into from
Jul 27, 2023

Conversation

gildub
Copy link
Contributor

@gildub gildub commented Jul 26, 2023

While resolving the related notification issues (see associated MTA-1024) ) this also refactors create/update modal and tries to bring more consistency.

Includes following changes :

  • Push edit/create notifications down to JobFunctionForm
  • Use onClose to replace onSave and onCancel
  • Replace REST functions to use axios instead of deprecated APIClient().
  • Use React Query mutations instead of legacy APIClient (axios) functions.
  • Consolidate New/Edit Modal duplication

Partially address https://issues.redhat.com/browse/MTA-1024

@gildub gildub changed the title ✨ [WIP] Jobfunctions: Update Notification and refactors update/create modal ✨ [WIP] Jobfunctions: Update Notification and refactors update/create modal and queries Jul 26, 2023
@gildub gildub changed the title ✨ [WIP] Jobfunctions: Update Notification and refactors update/create modal and queries ✨ Jobfunctions: Update Notification and refactors update/create modal and queries Jul 26, 2023
@gildub gildub self-assigned this Jul 26, 2023
@codecov
Copy link

codecov bot commented Jul 26, 2023

Codecov Report

Patch coverage: 60.00% and project coverage change: +0.01% 🎉

Comparison is base (ad327ba) 44.03% compared to head (6e434e1) 44.05%.
Report is 1 commits behind head on main.

❗ Current head 6e434e1 differs from pull request most recent head 09bb083. Consider uploading reports for the commit 09bb083 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1209      +/-   ##
==========================================
+ Coverage   44.03%   44.05%   +0.01%     
==========================================
  Files         177      177              
  Lines        4517     4515       -2     
  Branches     1007     1007              
==========================================
  Hits         1989     1989              
+ Misses       2517     2515       -2     
  Partials       11       11              
Flag Coverage Δ
unitests 44.05% <60.00%> (+0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Changed Coverage Δ
client/src/app/api/rest.ts 54.78% <60.00%> (+0.31%) ⬆️

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@ibolton336
Copy link
Member

ibolton336 commented Jul 26, 2023

Use React Query mutations instead of legacy queries

Is the new syntax really called RQ mutations? Seems like those are two distinct things: a query or a mutation. With a mutation modifying data on a server & a query just fetching data.

@gildub gildub requested review from ibolton336 and mturley and removed request for ibolton336 July 27, 2023 11:18
gildub added 3 commits July 27, 2023 16:15
Signed-off-by: Gilles Dubreuil <[email protected]>
Signed-off-by: Gilles Dubreuil <[email protected]>
Signed-off-by: Gilles Dubreuil <[email protected]>
@gildub gildub requested a review from ibolton336 July 27, 2023 14:16
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.

2 participants