Change url format #731
Change url format #731
Conversation
✅ Component tests succeed
|
✅ E2E tests succeed
|
sonic16x
force-pushed
the
users/rocketraccoon/TESTPLANE-778.url-refactoring
branch
from
e3afb87 to
007bb5e
Compare
commit: |
sonic16x
force-pushed
the
users/rocketraccoon/TESTPLANE-778.url-refactoring
branch
4 times, most recently
from
dc5409b to
101a293
Compare
sonic16x
force-pushed
the
users/rocketraccoon/TESTPLANE-778.url-refactoring
branch
from
101a293 to
12192fa
Compare
| step.args[0] as string, | ||
| currentResult?.attempt?.toString() as string | ||
| ].map(encodeURIComponent).join('/')); | ||
| if (step.type === StepType.Action && step.title === 'assertView') { |
There was a problem hiding this comment.
What does step.title === 'assertView' mean?
Why we didn't have this condition to navigate to suites page earlier and now we do?
| return currentSuite.id + ' ' + params.browser; | ||
| } | ||
|
|
||
| return null; |
There was a problem hiding this comment.
nit: This return null is redundant
| return result?.imageIds.map(imageId => state.tree.images.byId[imageId]) ?? []; | ||
| }; | ||
|
|
||
| export const getCurrentSuiteId = (params: Record<string, string | undefined>): ((state: State) => string | null) => { |
There was a problem hiding this comment.
I dont like how "getCurrentSuiteId" returns currentSuite.id + ' ' + params.browser
If its "getCurrentSuiteId", it should only return "currentSuite.id"
If it returns currentSuite.id + ' ' + params.browser, it should not be called "getCurrentSuiteId"
| const currentTreeNodeId = useMemo( | ||
| () => [currentImageSuiteId, currentImageStateName].join(' '), | ||
| [currentImageSuiteId, currentImageStateName] | ||
| ); |
There was a problem hiding this comment.
Whats the point of "useMemo" here?
Since its just a string and they are immutable, you wouldn't get any benefits from it being the same instance ("foo" === "foo")
I think, useMemo overhead is way more than just joining 2 strings
| export const getCurrentImageSuiteHash = (state: State): string | null => ( | ||
| state.tree.suites.byId[state.tree.browsers.byId[state.app.visualChecksPage.suiteId || '']?.parentId || '']?.hash | ||
| ); |
There was a problem hiding this comment.
So, you already have "suiteId" from "state.app.visualChecksPage.suiteId", but you are using it as a key to "state.tree.browsers.byId", which implies its browserId?
Then it should not be named "suiteId"
Also, it would be way more readable, if it would be written like this:
const suiteId = state.app.visualChecksPage.suiteId;
const browser = state.tree.browsers.byId[suiteId]; // Accessing browser by suiteId???
const suiteId = browser?.parentId || '' // Also suiteId?
return state.tree.suites.byId[suiteId].hash| if (pathname.startsWith(PathNames.suites)) { | ||
| return Page.suitesPage; | ||
| } | ||
|
|
||
| return Page.suitesPage; |
There was a problem hiding this comment.
Redundant check: whats the point of "pathname.startsWith(PathNames.suites)", if we return "Page.suitesPage" in both cases?
I would write this function like this:
return pathname.startsWith(PathNames.visualChecks) ? Page.visualChecksPage : Page.suitesPage;| if (page === Page.suitesPage) { | ||
| return PathNames.suites; | ||
| } | ||
|
|
||
| return PathNames.suites; |
There was a problem hiding this comment.
Redundant check: whats the point of "page === Page.suitesPage", if we return "PathNames.suites" in both cases?
I would write this function like this:
return page === Page.visualChecksPage ? PathNames.visualChecks : PathNames.suites;
Before we had URL format for suite page:
suites/${testId}/${state}/${attempt}and on visual checks page:
visual-checks/${imageId}/${attempt}imageIdhere istestId+ ' ' +stateI made common format:
${suites | visual-checks}/${hash}/${browser}/${attempt}/${state}