Skip to content

[Web] Fix scrolling in Node list in web terminal #26622

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.

Sign up for GitHub

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

Merged
rudream merged 1 commit into from
May 25, 2023
+25 −5
Merged

[Web] Fix scrolling in Node list in web terminal #26622

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions web/packages/teleport/src/Console/DocumentNodes/DocumentNodes.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -128,6 +128,7 @@ const Container = styled(Box)`
display: flex;
flex: 1;
max-width: 1024px;
height: fit-content;
Copy link
Copy Markdown
Member

@ravicious ravicious May 22, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you remember why overflow: hidden was added to StyledTableWrapper? If it's possible to get rid of it then I think long-term it might be a better fix than adding height: fit-content here.

Copy link
Copy Markdown
Contributor Author

@rudream rudream May 24, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reasoning was for all our different tables (paginated, searchable, simple, etc.) to have rounded corners. Wrapping all tables with it and having it round the corners with a borderRadius made it so we didn't have to always manually add a border radius for every table individually.

::after {
content: ' ';
padding-bottom: 24px;
Expand Down
1 change: 1 addition & 0 deletions ...ckages/teleport/src/Console/DocumentNodes/__snapshots__/DocumentNodes.story.test.tsx.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -739,6 +739,7 @@ exports[`render DocumentNodes 1`] = `
display: flex;
flex: 1;
max-width: 1024px;
height: fit-content;
}

.c2::after {
Expand Down
26 changes: 22 additions & 4 deletions web/packages/teleport/src/Sessions/SessionList/SessionJoinBtn.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,8 +35,14 @@ export const SessionJoinBtn = ({
participantModes: ParticipantMode[];
showCTA: boolean;
}) => {
const [anchorEl, setAnchorEl] = useState<HTMLElement>(null);
Copy link
Copy Markdown
Member

@ravicious ravicious May 22, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A feedback for the future as this pattern wasn't introduced by this PR specifically, but I don't think it's a good practice to store HTML elements in the state – you might end up storing an element in the state that's no longer in DOM. Was this done as a workaround for some other problem?

Looking at the previous version of JoinMenu, I think a more idiomatic solution would be to create a new ref with useRef, pass it as the ref prop to <ButtonBorder>. Then to control whether <Menu> is supposed to be open or not, we could have const [isOpen, setIsOpen] = useState(false) and on the button click we could flip it to true.

Copy link
Copy Markdown
Member

@ravicious ravicious May 22, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

MenuLogin does it this way already. It looks like the prop has to be named setRef for the button components, I'm not exactly sure why it's done like this.

teleport/web/packages/design/src/Button/Button.jsx

Lines 23 to 25 in 2bb3860

const Button = ({ children, setRef, ...props }) => {
return (
<StyledButton {...props} ref={setRef}>

Copy link
Copy Markdown
Contributor Author

@rudream rudream May 24, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed, I'll keep track of this and we can come back and refactor all our popover menu components at some point.


function closeMenu() {
setAnchorEl(null);
}

return (
<JoinMenu>
<JoinMenu anchorEl={anchorEl} setAnchorEl={setAnchorEl}>
{showCTA && (
<Box mx="12px" my="3">
<ButtonLockedFeature
Expand All @@ -56,6 +62,7 @@ export const SessionJoinBtn = ({
participantMode="observer"
key="observer"
showCTA={showCTA}
closeMenu={closeMenu}
/>
<JoinMenuItem
title="As a Moderator"
Expand All @@ -65,6 +72,7 @@ export const SessionJoinBtn = ({
participantMode="moderator"
key="moderator"
showCTA={showCTA}
closeMenu={closeMenu}
/>
<JoinMenuItem
title="As a Peer"
Expand All @@ -74,14 +82,21 @@ export const SessionJoinBtn = ({
participantMode="peer"
key="peer"
showCTA={showCTA}
closeMenu={closeMenu}
/>
</JoinMenu>
);
};

function JoinMenu({ children }: { children: React.ReactNode }) {
const [anchorEl, setAnchorEl] = useState<HTMLElement>(null);

function JoinMenu({
children,
anchorEl,
setAnchorEl,
}: {
children: React.ReactNode;
anchorEl: HTMLElement;
setAnchorEl: React.Dispatch<React.SetStateAction<HTMLElement>>;
}) {
const handleClickListItem = (event: React.MouseEvent<HTMLElement>) => {
setAnchorEl(event.currentTarget);
};
Expand Down Expand Up @@ -122,20 +137,23 @@ function JoinMenuItem({
participantMode,
url,
showCTA,
closeMenu,
}: {
title: string;
description: string;
hasAccess: boolean;
participantMode: ParticipantMode;
url: string;
showCTA: boolean;
closeMenu: () => void;
}) {
if (hasAccess && !showCTA) {
return (
<MenuItem
as="a"
href={url}
target="_blank"
onClick={closeMenu}
css={`
text-decoration: none;
padding: 8px 12px;
Expand Down
2 changes: 1 addition & 1 deletion web/packages/teleport/src/Sessions/SessionList/SessionList.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -112,7 +112,7 @@ const renderJoinCell = ({
showActiveSessionsCTA,
}: renderJoinCellProps) => {
const { joinable } = kinds[kind];
if (!joinable || participantModes.length === 0) {
Comment thread
rudream marked this conversation as resolved.
if (!joinable) {
return <Cell align="right" height="26px" />;
}

Expand Down