-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
Show available target ports in gateways for multi-port apps #51016
base: master
Are you sure you want to change the base?
Conversation
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.
LGTM, I left a small UX suggestion.
Available target ports:{' '} | ||
{props.tcpPortsAttempt.data.map(formatPortRange).join(', ')}. |
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.
What would you say for a small UX improvement? We could display the ports as buttons to make it easier to change the target port (plus it would be more obvious what these ports are for):
buttons.mov
Here is the code if you find this worth adding :)
{props.tcpPortsAttempt.status === 'success' && (
<Box>
Available target ports:
<Flex gap={1} flexWrap="wrap">
{props.tcpPortsAttempt.data.map(portRange => {
const formatted = formatPortRange(portRange);
return (
<ButtonSecondary
key={formatted}
onClick={() => {
const port = portRange.port.toString();
multiPortFieldRef.current.value = port;
changeTargetPort(port);
}}
size="small"
>
{formatted}
</ButtonSecondary>
);
})}
</Flex>
</Box>
)}
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.
That's a great idea, thanks. It didn't even occur to me because I was trying to keep it as simple as possible, since I've already spent too much time on this. ;f
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.
kind="warning" | ||
details={props.tcpPortsAttempt.statusText} | ||
primaryAction={{ content: 'Retry', onClick: props.getTcpPorts }} | ||
> |
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.
> | |
m=0 | |
> |
This PR adds a line under the form with ports with a list of available target ports. The list is fetched before the gateway is created, but only when the tab with the gateway is visible. This is so that if the user reopens a session with many app gateways open, we don't attempt to fetch all apps at once.
It's best to start the review at
web/packages/teleterm/src/ui/DocumentGatewayApp/DocumentGatewayApp.tsx
and then go deeper.