feat: implement UI to add experience to member #307 - #830
Conversation
Migration Test Results35 tests 35 ✅ 4s ⏱️ Results for commit 9c10359. |
Backend Test Results764 tests - 2 764 ✅ - 2 53s ⏱️ -5s Results for commit cd13716. ± Comparison against base commit a120187. This pull request removes 2 tests.♻️ This comment has been updated with latest results. |
|
@MasterEvarior |
Whatever is easier. In the end only the diff matters, as anything else does get squashed anyway. If you're not sure which one is easier, ask @schiltpuzzle or @lcanobbio. I am sure they can help you with that :) |
Thirusiga
changed the title
|
I revert it :) |
MasterEvarior
left a comment
MasterEvarior
left a comment
There was a problem hiding this comment.
This looks pretty good so far 🚀
I have some minor things to correct, a request for a new "feature" and some translation changes.
I have not yet looked at any tests, as they are currently still being written.
3d00912 to
494540c
Compare
cd13716 to
e237bea
Compare
| private DegreeType degreeType; | ||
|
|
||
| @NotNull(message = "{attribute.not.null}") | ||
| @PastOrPresent(message = "{attribute.date.past.present}") |
There was a problem hiding this comment.
why was this removed?
| private String comment; | ||
|
|
||
| @NotNull(message = "{attribute.not.null}") | ||
| @PastOrPresent(message = "{attribute.date.past.present}") |
| memberDetailPage.openModalButton('add', 'experience') | ||
| .click(); | ||
|
|
||
| modalPage.checkModalIconButtonVisible(); |
There was a problem hiding this comment.
this method not only checks if the button is visible but also whether the button is currently focused.
either rename the method or chanage the behavior as following:
the method should only return the icon button and the actual validation should be made in here
another idea would be to create a second method checkModalIconButtonFocused and then call both so the funtionality is instantly visible at the caller level
| cy.getByTestId('experienceType') | ||
| .type('Pra'); | ||
|
|
||
| cy.get('mat-option') |
There was a problem hiding this comment.
extract this into a method (smth. like selectFromAutocomplete) the method should take a fieldName , a string that should be entered in ithe input, and the name of the final element that should be selected
| modalPage.checkModalIsClosed(); | ||
| }); | ||
| }); | ||
| // test both alternative save buttons |
There was a problem hiding this comment.
if you really want to hang on to this comment put it at the top of the method
| comment: ['' as string | null] | ||
| }); | ||
|
|
||
| constructor() { |
There was a problem hiding this comment.
we did some neat simplifications in in a similar component in your last pr, look at it again and pull the changes in here
| import { AddDegreeComponent } from '../../degrees/add-degree/add-degree.component'; | ||
| import { DegreeService } from '../../degrees/degree.service'; | ||
|
|
||
|
|
| describe('isInteger', () => { | ||
| it('should return null if value is empty', () => { | ||
| const control = new FormControl(''); | ||
| expect(isInteger()(control)) |
There was a problem hiding this comment.
call the isInteger method only one time then store the validator in a var
then only call this return value (the var) so we can avoid this ugly ...()(...) syntax
| if (value === '') { | ||
| return null; | ||
| } | ||
| if ((/^-?\d+$/).test(value)) { |
There was a problem hiding this comment.
is there a significat advantage of a regex in comparision to a number parsing and check if Number is not NaN
| modalSubmitMode: ModalSubmitMode.SAVE, | ||
| submittedModel: { | ||
| ...experience1, | ||
| completed: true |
There was a problem hiding this comment.
where is this completed true coming from?
cause either it's not needed (we set the value of the form to the same var a few lines above)
otherwise there is some mechanism that updates that property that isn't the user and i want to know why
3 participants
No description provided.