PB-2064: reportproblem cypress #1504
Conversation
sommerfe
force-pushed
the
fix-pb-2064-reportproblem-cypress
branch
from
330f582 to
ed0d7b9
Compare
sommerfe
force-pushed
the
fix-pb-2064-reportproblem-cypress
branch
4 times, most recently
from
feffc53 to
4507978
Compare
sommerfe
force-pushed
the
fix-pb-2064-reportproblem-cypress
branch
from
d7a8296 to
b098f29
Compare
| /** Flag to indicate if the user is currently reporting a problem with the drawing */ | ||
| reportProblemDrawing: boolean |
There was a problem hiding this comment.
I have a feeling that online was exactly there to cover this use-case.
Can you check if you can get by only by setting/reading drawingStore.online (with maybe an option to set it correctly when calling drawingStore.initiateDrawing if it doesn't exist yet)?
Here we are bringing business names and logic (report a problem) that has nothing to do with the drawing per-say, I'd like to keep that separated as much as possible
There was a problem hiding this comment.
The problem i was facing was when we do first report problem drawing and before submitting, create a normal drawing. Or the other way around first normal drawing and then report problem drawing. In this case the online variable is not sufficient, because we then loose track of the fact that we have a report problem drawing which is not submitted yet.
There was a problem hiding this comment.
Then we should change how initiateDrawing works, and be able to give there the "on-going" drawing from the report a problem.
If we could keep the drawing module the most oblivious to the rest of the app, the better we will be able to make a module out of it down the line
There was a problem hiding this comment.
ok i can have a look at it and see if i can find a simpler solution for it
There was a problem hiding this comment.
@pakb I refactored the boolean online into an enum to hold the different states we have, please check if it is ok like this
| validate = undefined, | ||
| dataCy = '', | ||
| } = defineProps<{ | ||
| const props = withDefaults(defineProps<{ |
There was a problem hiding this comment.
validMarker and invalidMarker are already flagged with ? (optional), I don't think it's required to set a defautl value to them anyway.
If it's the case because the TS type-checking complains otherwise, I'd not use withDefaults, but do a two phase deconstruct.
const props = defineProps<...>()
const { propsNeedingDefault = undefined } = props
// don't do a full `toRefs` on all the props (it will wrap some that we don't require) but specifically ref some
const label = toRef(props, 'label' /* eventual default value here as 3rd arg */)
// ...There was a problem hiding this comment.
i tried using this, but all the reactivity didn't work anymore, this is what copilot told me why it is not working
The problem is that in TypeScript/Vue 3, when you define optional props without withDefaults, they default to undefined when not provided. However, boolean props have special behavior in Vue:
When you don't use withDefaults and a boolean prop is not provided, it's undefined
But Vue's prop system can sometimes treat missing boolean props as false instead of undefined
I checked and the values are really evaluated as false, and i think this is killing the reactivity because when they change value to false e.g. it is not passed along since the value was already false before
| force: true, | ||
| }) | ||
| cy.get('@submit').click() | ||
| cy.get('[data-cy="file-input-text"]').should('have.class', 'is-valid') |
There was a problem hiding this comment.
wasn't that some valid testing? Why remove it?
There was a problem hiding this comment.
When the submitting worked, the file-input-text is not there anymore, the report problem modal just shows a confirmation of the submitting message. Not sure why it worked before
| cy.log('Write description and email') | ||
| cy.get('[data-cy="report-problem-text-area"] [data-cy="text-area-input"]') | ||
| .as('textArea') | ||
| .scrollIntoView() |
There was a problem hiding this comment.
AFAIK all the call to .scrollIntoView were there to help the CI that was a bit flaky there, I don't know if it's a good idea to remove it
There was a problem hiding this comment.
In my tests the .scrollIntoView did not work and without it it worked
Test link