Skip to content
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

fix(agent): action menu should have flip enabled #584

Merged
merged 1 commit into from
Dec 5, 2022

Conversation

tthvo
Copy link
Member

@tthvo tthvo commented Oct 27, 2022

Fixes #583
Fixed #595 (this PR includes the final refactoring piece).

Refactored probe template table to use custom action menu that enables flipping when viewport boundary is reached.

@tthvo tthvo added the fix label Oct 27, 2022
@tthvo
Copy link
Member Author

tthvo commented Oct 27, 2022

This is more of a hack. We could do better to configure the menu to open up if there is not enough space below. Not sure if there is a way now with IAction. We might need to create a manual popover as recording table.

@andrewazores
Copy link
Member

I was hoping there was a nice way to do this with the dropdown menu opening upward when needed, but it doesn't seem possible. The only thing is this:

      <Table
        aria-label="Actions table"
        cells={columns}
        rows={rows}
        {...(propToUse === 'actions' && { actions })}
        {...(propToUse === 'actionResolver' && { actionResolver })}
        areActionsDisabled={rowData => !!rowData.disableActions}
        dropdownPosition="left"
        dropdownDirection="down"
        actionsToggle={useCustomToggle ? customActionsToggle : undefined}
      >

(https://www.patternfly.org/v4/components/table/react-legacy)

Forcing this dropdown to always open upward seems a little weird. Making the margins larger or just adding some bottom padding to the card seems probably better.

@tthvo
Copy link
Member Author

tthvo commented Oct 27, 2022

Ahh that unfortunate :((. Are we okayy with 1em for bottom margin of the card? Though if later we add another option to the action, we might need to add more or refactor to use custom action menu.

@andrewazores
Copy link
Member

andrewazores commented Oct 27, 2022

I think a little more bottom margin would be good. Maybe leave the margin-top styling and add a margin-bottom: 2em or so, or maybe padding to the card so that there's more space between the end of the table and the end of the card and the menu stays more on the card instead of hanging off the edge.

Fixing the menu item to dynamically pop upward would be even better but that's a lot more work for a single small display issue that has an easy workaround aready.

@tthvo
Copy link
Member Author

tthvo commented Oct 28, 2022

I think a little more bottom margin would be good. Maybe leave the margin-top styling and add a margin-bottom: 2em or so, or maybe padding to the card so that there's more space between the end of the table and the end of the card and the menu stays more on the card instead of hanging off the edge.

Fixing the menu item to dynamically pop upward would be even better but that's a lot more work for a single small display issue that has an easy workaround aready.

Right... okayy I refactored the table to use the new TableComposable, which allows custom action menu. The action menu is implemented as a Dropdown with isFlipEnabled={true} and position={DropdownPosition.right} (i.e. open to left). Snapshot has to be dropped due to Popper depends on DOM.

The menu is flipped vertically as expected. However, the horizontal positioning to the right is not applied. It took a while to find out that Patternfly team actually filed a bug here: patternfly/patternfly-react#8013 (2022.14). This does not look like we can upgrade Patternfly in time for our 2.2.0 as Patternfly release note is only up to 2022.13.

I guess we have to be on blocking for now.

@tthvo
Copy link
Member Author

tthvo commented Oct 28, 2022

Also on this note:

I refactored the table to use the new TableComposable

I think we should convert all legacy tables to use this new component so that we can use new features later.

@andrewazores
Copy link
Member

Nice work with the refactoring. Could you open a separate PR to just add the bottom margin then so we can get that merged in the short term, and hopefully for the next release we replace it with this PR and an updated Patternfly version?

@tthvo
Copy link
Member Author

tthvo commented Oct 28, 2022

Nice work with the refactoring. Could you open a separate PR to just add the bottom margin then so we can get that merged in the short term, and hopefully for the next release we replace it with this PR and an updated Patternfly version?

Oh right! I opened #590 now :D

@tthvo tthvo force-pushed the agent-ui branch 2 times, most recently from 562faca to 8b32681 Compare November 10, 2022 18:31
@tthvo tthvo changed the title fix(agent): action menu should not be out of viewport fix(agent): action menu should have flip enabled Nov 10, 2022
@tthvo tthvo force-pushed the agent-ui branch 2 times, most recently from ae24c05 to 584a5a2 Compare December 5, 2022 22:24
@tthvo
Copy link
Member Author

tthvo commented Dec 5, 2022

Seems like the fix in this Patternfly core is already applied. But the menu is not flipping correctly, for some reason, because the Dropdown was placed within a Td. I will take a closer look and might have to go with another way.

Edit: Actually, we just need to append the menu to document.body instead of parent. Though, this is discouraged for accessibility support.

@tthvo
Copy link
Member Author

tthvo commented Dec 5, 2022

The menu is now correctly flipped. Should be ready for review again :D

@tthvo tthvo removed the blocked label Dec 5, 2022
Signed-off-by: Thuan Vo <thvo@redhat.com>
@andrewazores
Copy link
Member

Edit: Actually, we just need to append the menu to document.body instead of parent. Though, this is discouraged for accessibility support.

Are the other changes still related/required then?

@tthvo
Copy link
Member Author

tthvo commented Dec 5, 2022

Edit: Actually, we just need to append the menu to document.body instead of parent. Though, this is discouraged for accessibility support.

Are the other changes still related/required then?

Oh yes they are required. Sorry, I mean we need to set document.body for the new AgentTemplateAction defined here instead of parent previously before upgrading patternfly/core.

@tthvo tthvo added the chore Refactor, rename, cleanup, etc. label Dec 5, 2022
@andrewazores andrewazores merged commit 58d0aaa into cryostatio:main Dec 5, 2022
@tthvo tthvo deleted the agent-ui branch June 27, 2023 08:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
chore Refactor, rename, cleanup, etc. fix
Projects
No open projects
Status: Done
2 participants