-
Notifications
You must be signed in to change notification settings - Fork 369
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
Api access methods desktop GUI #5714
Conversation
143ef73
to
9d19ce1
Compare
be8081a
to
5fd3236
Compare
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.
Reviewed 36 of 36 files at r1, 2 of 2 files at r2, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @raksooo)
gui/src/renderer/components/ContextMenu.tsx
line 162 at r1 (raw file):
window.innerHeight ? 'down' : 'up';
nit this computation looks a bit hairy due to it being split up across multiple lines, but I get if you don't want to introduce intermediate variables / extract into a function either. If you feel like it's readable as is, simply ignore this nit
Code quote:
return bounds.y +
bounds.height +
props.items.length * ITEM_HEIGHT +
2 * (BORDER_WIDTH + PADDING_VERTICAL) <
window.innerHeight
? 'down'
: 'up';
gui/src/renderer/components/Modal.tsx
line 106 at r1 (raw file):
warning, working,
May I suggest something like loading
/progressing
/inProgress
? working
can be mistaken for a final state, in which case it could be confused with the success
variant of this enum 😊
Code quote:
working
gui/src/renderer/components/SmallButton.tsx
line 20 at r1 (raw file):
}; case SmallButtonColor.blue: default:
nit: Not necessary to switch on SmallButtonColor.blue
since it just falls through to default
, but I see how it can be nice for explicitness 😊
Code quote:
case SmallButtonColor.blue:
default:
gui/src/renderer/components/cell/SettingsRadioGroup.tsx
line 16 at r1 (raw file):
function getGroupKey() { return ++groupCounter; }
Could the useId
hook be used here as well? 😊
Code quote:
let groupCounter = 0;
function getGroupKey() {
return ++groupCounter;
}
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.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @raksooo)
gui/src/renderer/lib/api-access-methods.ts
line 12 at r1 (raw file):
function getTestId() { return ++testId; }
Could the useId
hook be used here as well? 😊
Code quote:
// Each test needs to have an id to be cancelable.
let testId = 0;
function getTestId() {
return ++testId;
}
07c909b
to
b8f8b87
Compare
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.
I've fixed your comments and also added a line to the changelog.
Reviewable status: 30 of 37 files reviewed, 2 unresolved discussions (waiting on @MarkusPettersson98)
gui/src/renderer/components/ContextMenu.tsx
line 162 at r1 (raw file):
Previously, MarkusPettersson98 (Markus Pettersson) wrote…
nit this computation looks a bit hairy due to it being split up across multiple lines, but I get if you don't want to introduce intermediate variables / extract into a function either. If you feel like it's readable as is, simply ignore this nit
That was horrible 😆 How did I not notice this beast myself when writing it 🙈 Fixed!
gui/src/renderer/components/Modal.tsx
line 106 at r1 (raw file):
Previously, MarkusPettersson98 (Markus Pettersson) wrote…
May I suggest something like
loading
/progressing
/inProgress
?working
can be mistaken for a final state, in which case it could be confused with thesuccess
variant of this enum 😊
Loading sounds great to me! Done.
gui/src/renderer/components/SmallButton.tsx
line 20 at r1 (raw file):
Previously, MarkusPettersson98 (Markus Pettersson) wrote…
nit: Not necessary to switch on
SmallButtonColor.blue
since it just falls through todefault
, but I see how it can be nice for explicitness 😊
When I wrote it I opted for explicitness but when thinking about it now I realize default
is explicit enough. Fixed!
gui/src/renderer/components/cell/SettingsRadioGroup.tsx
line 16 at r1 (raw file):
Previously, MarkusPettersson98 (Markus Pettersson) wrote…
Could the
useId
hook be used here as well? 😊
Yes, done!
gui/src/renderer/lib/api-access-methods.ts
line 12 at r1 (raw file):
Previously, MarkusPettersson98 (Markus Pettersson) wrote…
Could the
useId
hook be used here as well? 😊
Unfortunately not. Hooks can only be used at the top level in components/custom hooks and this is used in a callback. Although when taking a look at it I realized we could use the actual promise to compare if it's the most recent test or not.
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.
Reviewed 7 of 7 files at r3, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved
gui/src/renderer/components/ContextMenu.tsx
line 162 at r1 (raw file):
Previously, raksooo (Oskar Nyberg) wrote…
That was horrible 😆 How did I not notice this beast myself when writing it 🙈 Fixed!
nice 🎉
gui/src/renderer/lib/api-access-methods.ts
line 12 at r1 (raw file):
Previously, raksooo (Oskar Nyberg) wrote…
Unfortunately not. Hooks can only be used at the top level in components/custom hooks and this is used in a callback. Although when taking a look at it I realized we could use the actual promise to compare if it's the most recent test or not.
I see, but your new solution seems fins to me! 😊
73714e5
to
9a1f8ce
Compare
9a1f8ce
to
4776f28
Compare
This PR adds the API access method GUI (https://app.zeplin.io/project/5f928a32fdc9962af9018d70/flow/61f14327d6d7cda2828d3f0c), along with a few new components for handling the new form layout and input types:
Fixes DES-341, DES-555
This change is