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.

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

Jump to bottom

Wizard not in modal #1327

Merged
merged 1 commit into from
Oct 9, 2023
Merged

Wizard not in modal #1327

merged 1 commit into from
Oct 9, 2023

Conversation

lavocatt
Copy link
Contributor

@lavocatt lavocatt commented Aug 23, 2023 •
edited
Loading

This pull requests makes the wizard appear in its own page when the user clic on the "create an image" button.
However, there's a bug with the height of the wizard still being limited, see the bug opened on data driven forms data-driven-forms/react-forms#1402

To overcome the bug a custom CSS rule deactivate the height limitation.

@lavocatt
Copy link
Contributor Author

Still "blocked" by the fact that the height of the wizard is limited. But I don't know, maybe we can merge this one anyway ? I think the benefit of having access to the quickstart outweighs the downside of this bug. What's your opinion @lucasgarfield ?

@lavocatt
Copy link
Contributor Author

I think I can also fix this with two simple CSS rules.

@codecov
Copy link

codecov bot commented Sep 19, 2023 •
edited
Loading

Codecov Report

Merging #1327 (c6cea1e) into main (cd2d3c4) will not change coverage.
The diff coverage is n/a.

❗ Current head c6cea1e differs from pull request most recent head b8d9be3. Consider uploading reports for the commit b8d9be3 to get more accurate results

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #1327   +/-   ##
=======================================
  Coverage   73.30%   73.30%           
=======================================
  Files          70       70           
  Lines        2000     2000           
  Branches      529      529           
=======================================
  Hits         1466     1466           
  Misses        483      483           
  Partials       51       51
Files Coverage Δ
.../Components/CreateImageWizard/CreateImageWizard.js 66.10% <ø> (ø)
src/Router.js 0.00% <ø> (ø)

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update cd2d3c4...b8d9be3. Read the comment docs.

@lavocatt lavocatt changed the title [WIP] Wizard not in modal Wizard not in modal Sep 19, 2023
@lavocatt
Copy link
Contributor Author

DDF making the calls to PF4 it turns out we're quite limited in the options we have to style the wizard.

@lavocatt lavocatt mentioned this pull request Sep 25, 2023
Closed
@lavocatt lavocatt force-pushed the wizard-not-in-modal branch 2 times, most recently from 9166c51 to 93b550c Compare October 9, 2023 08:11
@lavocatt
Copy link
Contributor Author

lavocatt commented Oct 9, 2023

We've decided with @lucasgarfield to get the wizard in page without waiting for DDF to be removed and to add custom CSS rules in the meantime to cover for the things that are blocking us. The main reason why it's important is that the current wizard in modal suffers from flickering, and going in page solves this issue.

@lavocatt lavocatt requested a review from regexowl October 9, 2023 08:35
@lavocatt lavocatt force-pushed the wizard-not-in-modal branch 2 times, most recently from 2b0276d to f08c491 Compare October 9, 2023 08:59
regexowl
regexowl approved these changes Oct 9, 2023
View reviewed changes
Copy link
Collaborator

@regexowl regexowl left a comment

Choose a reason for hiding this comment

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

Looks good! Also no more re-renders on opening the modal 🤩

@lavocatt
wizard: don't show the form as a modal
b8d9be3 
When the user clicks on the "create an image" button, the image wizard
shows up replacing the landing page. This allows to keep the quickstart
guides to the right of the wizard while the user is interacting with it.
@lavocatt lavocatt enabled auto-merge (rebase) October 9, 2023 09:32
@lavocatt lavocatt merged commit cbe710e into osbuild:main Oct 9, 2023
1 of 2 checks passed
@regexowl regexowl mentioned this pull request Nov 2, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@regexowl regexowl regexowl approved these changes

@lucasgarfield lucasgarfield Awaiting requested review from lucasgarfield

Assignees
No one assigned
Labels
✅ ready for review
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@lavocatt @regexowl