Skip to content

Conversation

@leSamo
Copy link
Contributor

@leSamo leSamo commented Apr 12, 2024

Column Management Modal

Associated ticket: RHINENG-3003

What

Implement a modal that a user can use to configure the visibility of individual table columns. The modal supports columns that are hidden by default and also those that should be unhideable.

image

Screencastfrom2024-04-1216-04-18-ezgif.com-video-to-gif-converter.webm

Why

Since there is a request (see the associated ticket above) to implement column management in tables in all apps across Insights it would make sense to abstract a component that could be reused. Currently, the only implementation of column management is in Insights Vulnerability (hidden inside kebab menu in table toolbar). This PR is based on the Vulnerability's implementation inspired by the PF column management demo.

Example usage in practice with Redux

// Hooks file
export const useColumnManagement = (columns, onApply) => {
    const [isModalOpen, setModalOpen] = useState(false);

    return [(
        <ColumnManagementModal
            appliedColumns={columns}
            applyColumns={newColumns => onApply(newColumns)}
            isModalOpen={isModalOpen}
            setModalOpen={setModalOpen}
        />), setModalOpen];
};

// Table file
const columns = useSelector(({ TableStore }) => TableStore.columns;
const [ColumnManagementModal, setColumnManagementModalOpen] = 
    useColumnManagement(columns, newColumns => dispatch(changeTableAction(newColumns)));

return (
    ...
    <Button onClick={() => setColumnManagementModalOpen(true)}>Manage columns</Button>
    ...
    { ColumnManagementModal }
    ...
)

@patternfly-build
Copy link

patternfly-build commented Apr 12, 2024

@Hyperkid123 Hyperkid123 requested review from dlabaj and fhlavac April 17, 2024 07:39
Copy link
Contributor

@fhlavac fhlavac left a comment

Choose a reason for hiding this comment

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

@leSamo thank you for your contribution. It looks really good! I've added some comments. Some of them may be worth discussion. Please let me know what do you think or if you need any help 🙂

Copy link

@andrew-ronaldson andrew-ronaldson left a comment

Choose a reason for hiding this comment

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

The only thing I am noticing is the modal buttons. There was a decision on PF to make the Cancel button be a plain text button rather than the secondary button styling. I found that our Managed columns react demo was not updated so I created an issue to make that change on our PatternFly site.

Screenshot is a v6 example but the Cancel button is shown accurately.
Screenshot 2024-04-18 at 12 27 32 PM

@leSamo leSamo marked this pull request as draft April 18, 2024 23:01
@leSamo leSamo force-pushed the column-mgmt-modal branch from 03d14ef to 556fa4b Compare April 23, 2024 21:04
@leSamo leSamo requested a review from fhlavac April 23, 2024 21:35
@leSamo leSamo marked this pull request as ready for review April 23, 2024 21:35
@leSamo
Copy link
Contributor Author

leSamo commented Apr 23, 2024

Thank you @fhlavac for the comprehensive review, I have (hopefully) addressed all of your and @ andrew-ronaldson's comments.

…roups/examples/ColumnManagementModal/ColumnManagementModal.md

Co-authored-by: Filip Hlavac <[email protected]>
@leSamo leSamo requested a review from fhlavac April 24, 2024 09:13
…roups/examples/ColumnManagementModal/ColumnManagementModal.md
@fhlavac
Copy link
Contributor

fhlavac commented Apr 24, 2024

@andrew-ronaldson can you please take a look? So this one can be merged

@andrew-ronaldson
Copy link

Looks good! I think the only comment I have leans more into the design guidelines. The example given includes a primary button as the trigger for this modal. That's fine on its own but if the table has a toolbar with other actions we should only have one primary button on the page. In OCP we had these buttons to trigger the manage column modal.

If I want to use this code in my product, can I easily change that primary button to a secondary or icon button? If that's the case I think we just need a bit of text pointing to design guidelines for buttons.

Screenshot 2024-04-24 at 10 09 26 AM

@fhlavac
Copy link
Contributor

fhlavac commented Apr 24, 2024

@andrew-ronaldson The primary button is just part of the example, not the component itself, so the user can use any trigger he wants. There is probably no need for this trigger to be part of the component. But perhaps we could update our examples to show the proposed trigger compliant with the guidelines? @leSamo what do you think?

@leSamo
Copy link
Contributor Author

leSamo commented Apr 24, 2024

@andrew-ronaldson I changed the example button variant to secondary and added an icon to the example. The button is not part of the component, since there are various ways to implement it, I think the majority of the time the button will be inside of a kebab menu. Is this okay?

Screenshot from 2024-04-24 22-17-08

Copy link

@andrew-ronaldson andrew-ronaldson left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@fhlavac fhlavac merged commit 7f9adb9 into patternfly:main Apr 30, 2024
@github-actions
Copy link

🎉 This PR is included in version 5.2.0-prerelease.3 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants