-
Notifications
You must be signed in to change notification settings - Fork 43
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
create new tag #658
Merged
Merged
create new tag #658
Changes from all commits
Commits
Show all changes
14 commits
Select commit
Hold shift + click to select a range
0d1802a
create new tag
daxter44 f21644b
add notification after create tag
daxter44 721633d
Catch SDK exception in tags create
ad-m 9d6bb84
fix strings
daxter44 41da224
add locales to notifications
daxter44 a698cd8
fix tags list path
daxter44 519fe3d
improvement of comments
daxter44 510359e
improvement of comments
daxter44 6f7f7fe
Add POC for field validation
ad-m 93dd88d
show api response in form
daxter44 fddfe84
fix errors displaying
daxter44 4a527dc
change createTag property name to tagState
daxter44 ecc6a69
fix error map on submit
daxter44 2442d64
Merge branch 'dev' into create-new-tag
daxter44 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,59 @@ | ||
import { create } from '@/services/tags'; | ||
import { Effect } from 'dva'; | ||
import { Reducer } from 'redux'; | ||
import { router } from 'umi'; | ||
import { openNotificationWithIcon } from '@/models/global'; | ||
import { formatMessage } from 'umi-plugin-react/locale'; | ||
import { Tag } from '@/services/definitions'; | ||
|
||
export interface TagModelState { | ||
status?: number; | ||
tag?: Tag; | ||
} | ||
|
||
export interface TagModelType { | ||
namespace: string; | ||
state: TagModelState; | ||
effects: { | ||
create: Effect; | ||
}; | ||
reducers: { | ||
changeTagStatus: Reducer<TagModelState>; | ||
}; | ||
} | ||
const TagState: TagModelState = { status: 1, tag: { name: '', id: 0 } }; | ||
const TagModel: TagModelType = { | ||
namespace: 'tag', | ||
state: TagState, | ||
effects: { | ||
*create({ payload }, { call, put }) { | ||
try { | ||
const response = yield call(create, payload); | ||
yield put({ | ||
type: 'changeTagStatus', | ||
payload: response, | ||
}); | ||
openNotificationWithIcon( | ||
'success', | ||
formatMessage({ id: 'tags-new.page-create-notiffy-success' }) + response.data.id, | ||
); | ||
router.replace(`/tags/list`); | ||
} catch (err) { | ||
yield put({ | ||
type: 'changeTagStatus', | ||
payload: err, | ||
}); | ||
} | ||
}, | ||
}, | ||
reducers: { | ||
changeTagStatus(state, { payload }) { | ||
return { | ||
...state, | ||
status: payload.response.status, | ||
tag: payload.response.body, | ||
}; | ||
}, | ||
}, | ||
}; | ||
export default TagModel; |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,7 +1,9 @@ | ||
export default { | ||
'tags-new.form.save.label': 'Save', | ||
'tags-new.form.name.label': 'Name', | ||
'tags-new.form.name.placeholder': "Case's name", | ||
'tags-new.form.name.placeholder': "Tag's name", | ||
'tags-new.form.name.required-error': 'Name is required', | ||
'tags-new.page-header-content': 'You can add new channel here.', | ||
'tags-new.page-header-content': 'You can add new tag here.', | ||
'tags-new.page-create-notiffy-error': 'Save error:', | ||
'tags-new.page-create-notiffy-success': 'Tag saved corectly ID:', | ||
}; |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Czy możemy to zapisać generycznie, aby błędy w polach wyświetlały się przy polach formularzach, zamiast w powiadomieniu bez informacji jakiego pola dotyczy błąd?
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.
Ja nie widzę problemu, aby dopracować strukturę odpowiedzi błędu API, ale potrzebne wytyczne co do potrzeb.
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.
@ad-m W oknie logowania, mamy wyświetlanie problemu w formularzu (pomiedzy "Hasło logowania do konta" a polem z loginem ) I gdybym posiedział to jestem w stanie jeszcze to odwzorować.
Niestety nie wiem jak mogła by wyglądać struktura odpowiedzi z api aby przypisywać błąd do konkretnego pola w formularzu. Pozwolę sobie zawołać @kuskoman, czy jesteś w stanie coś tutaj zasugerować ?
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.
Obecna odpowiedź API wskazuje konkretne pole, bo masz
name
. Ja proponuje starać się użyć obecnej odpowiedzi, a jak problem zostanie rozpracowany mocniej (więcej przypadków) to zmienimy strukturę API, aby uprościć implementacje w front-endzie. Wykonamy to przed wejściem z V2 na produkcje.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.
@ad-m siedziałem trochę godzin nad zagadnieniem, próbowałem szukać pomocy na forach jednak nie udało mi się zrobić odpowiedzi według Twoich założeń. Dokładniej mam problem z przekazaniem odpowiedzi z Modelu do formularza, próbowałem odgapić to z elementu logowania bądź z Cases/New (ponieważ są tam zwracane kolekcje tagów/instytucji itd.) jednak nic z tego.
Odniosę się tutaj do reszty issue, chętnie się za nie wezmę gdy ktoś opracuje standard edycji/dodawania/usuwania ponieważ tak jak mówiłem na pierwszym call'u to moje początki z reactem i reduxem i dopiero poznaje mechanikę ich działania :)
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.
Ja w zakresie Reacta nie mam zbytniego doświadczenia także, ale podglądając zamykając projekt w Vue, z którym mam zaproponowałem:
Teraz pozostaje tylko rozważenie gdzie ta logika obsługi błędów będzie się znajdować, czy w komponencie strony (jak teraz wkleiłem), czy w modelu (jak teraz jest), czy w serwisie (jak mogłoby teoretycznie być), a następnie odpowiednie przekazanie danych. Proszę o komentarz @fervero i @mateuszmagulski , którzy mają większe doświadczenie w projekcie.
Jeżeli będziemy mieli ten element poprawnie rozwiązany to możemy znacznie więcej walidacji pchnąć na back-end (na początku nawet nie musimy tracić czasu na komunikat dla wymaganego pola, bo back-end to sprawdzi). W zespole obecnie nie widzę osoby, która po prostu może wyznaczyć standard, więc raczej nie unikniemy tego.
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.
@ad-m Ale taka walidacja na próbę zapisu pustego pola to była...
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.
Ale mój kod nie odnosi się wyłącznie do wymaganego pola :) Kod może przetwarzać także pozostałe błędy z back-endu (nieprawidłowy format, złą długość itd.). Poza tym jest gwarantowane, że będzie to spójne z back-endem. Akurat tym razem przypadek nam się powtórzył, ale patrz szerzej jako na standard.
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.
Tylko wąski zakres błędów jesteś w stanie sprawdzić po stronie front-endu.
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.
Dowiozłem to ! Po wysłaniu tag'a treść błędu dotycząca pola name jest w: props.createdTag.tag?.name