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

Refactor DiscoveryImageSummary to DownloadIsoModal #604

Merged

Conversation

mareklibra
Copy link
Contributor

@mareklibra mareklibra commented May 17, 2021

Depends on:

@mareklibra mareklibra force-pushed the refactorToDownloadIsoModal branch 3 times, most recently from eb97b5e to ca9d4e1 Compare May 18, 2021 11:02
@mareklibra mareklibra force-pushed the refactorToDownloadIsoModal branch from ca9d4e1 to ff6d76f Compare May 18, 2021 11:23
@mareklibra mareklibra changed the title WIP Refactor DiscoveryImageSummary to DownloadIsoModal Refactor DiscoveryImageSummary to DownloadIsoModal May 18, 2021
@mareklibra mareklibra mentioned this pull request May 20, 2021
1 task
isOpen: boolean;
};

const DownloadIsoModal: React.FC<DownloadIsoModalProps> = ({ isOpen, ...props }) => (

Choose a reason for hiding this comment

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

Related to https://github.com/rawagner/dynamic-cim/pull/4/files/7df6d034c9670fb111ca7d47b34c1635a6d10b90#r638496136 Should we use modals from assisted-ui-lib or handle it on the console side?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Commented there and on the slack. IMO, it would bring unnecessary complexity to provide that feature for both modal and non-modal usa-cases. The outer context still drives modal-related primitives and visibility (isOpen, onClose).

@mareklibra mareklibra merged commit c861b6a into openshift-assisted:master May 26, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants