Skip to content

Fix(Layout Shift Fix) - Resolved Layout Issues on DarkMode Toggle Button and Full-Screen Modal#125

Merged
DianaLease merged 6 commits intoaccordproject:mainfrom
nitro56565:nitro56565/layout-and-UI-fix
Mar 5, 2025
Merged

Fix(Layout Shift Fix) - Resolved Layout Issues on DarkMode Toggle Button and Full-Screen Modal#125
DianaLease merged 6 commits intoaccordproject:mainfrom
nitro56565:nitro56565/layout-and-UI-fix

Conversation

@nitro56565
Copy link
Contributor

@nitro56565 nitro56565 commented Feb 18, 2025

Summary

This PR addresses layout shift issues affecting the DarkToggle Button and Full-Screen Modal, ensuring a smoother and more consistent UI experience.

Changes

  • Fixed unintended layout shift on Full screen modal while on start guide tour.
  • Adjusted positioning of the Darkmode button to prevent UI flickering and unnecessary clicks.

Screenshots or Video

Before

Screenshot From 2025-02-18 22-17-22
Screenshot From 2025-02-18 22-19-50

After

Screenshot From 2025-02-24 20-50-26
Screenshot From 2025-02-24 20-51-13
Screenshot From 2025-02-24 20-51-26

Author Checklist

  • Ensure you provide a DCO sign-off for your commits using the --signoff option of git commit.
  • Vital features and changes captured in unit and/or integration tests
  • Commits messages follow AP format
  • Extend the documentation, if necessary
  • Merging to main from fork:branchname

@nitro56565 nitro56565 requested a review from a team as a code owner February 18, 2025 17:26
@netlify
Copy link

netlify bot commented Feb 18, 2025

Deploy Preview for ap-template-playground ready!

Name Link
🔨 Latest commit ecefd34
🔍 Latest deploy log https://app.netlify.com/sites/ap-template-playground/deploys/67c068003b0d8e0008a0b9a4
😎 Deploy Preview https://deploy-preview-125--ap-template-playground.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link
Member

@DianaLease DianaLease left a comment

Choose a reason for hiding this comment

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

I wonder if the tutorial should actually be targeting the "Preview Output" header in that step instead, rather than the full screen toggle button.

@nitro56565
Copy link
Contributor Author

In the start tour, we generally cover the options, features, and usage of the playground. When David implemented the start tour, the full-screen modal was already a part of it. If I’m not mistaken, if you’re referring to including the entire header instead of just the full screen modal or dark mode toggle button, I can definitely work on clubbing it together.

@nitro56565
Copy link
Contributor Author

@DianaLease I have updated the layout by combining the preview output with the two feature buttons: Dark Mode and Full-Screen Modal. Additionally, I have added a step in the start tour for the Dark Mode button, as it was missing before.

@nitro56565 nitro56565 force-pushed the nitro56565/layout-and-UI-fix branch from 9ed9c56 to b2330b2 Compare February 21, 2025 06:51
@sanketshevkar
Copy link
Member

Toggle button inside the preview block seems a bit off to me.

Scope of the toggle button is for the whole app and not just the preview block and the exapnd button's scope is just on the preview block. I don't feel they should be clubbed together.

@nitro56565
Copy link
Contributor Author

so do you want to handle the toggle button to be outside the scope and the expand button inside the scope?

@nitro56565
Copy link
Contributor Author

@DianaLease @sanketshevkar , can you review this layout now? If it looks good, I will write the test case for it.

@DianaLease
Copy link
Member

@nitro56565 This looks good, although can you target the "Preview Output" header here instead of the expand button?
Screenshot From 2025-02-24 20-51-13

@nitro56565
Copy link
Contributor Author

nitro56565 commented Feb 25, 2025

@DianaLease Do you want me to target just the header, or would the whole preview window be a better choice?

Screenshot From 2025-02-25 22-59-50
Screenshot From 2025-02-25 23-02-27
Screenshot From 2025-02-25 23-04-38

@sanketshevkar
Copy link
Member

sanketshevkar commented Feb 26, 2025

NIT: The dialog box is merging. Is it possible if we could undo that? Rest looks good.

Also please fix the failing test.

@nitro56565
Copy link
Contributor Author

NIT: The dialog box is merging. Is it possible if we could undo that? Rest looks good.

Also please fix the failing test.

sure @sanketshevkar I can handle the merging issue but from the above 3 options which one do you prefer for preview output, I believe the whole window would be a good option as referred below as well.

Screenshot From 2025-02-25 22-59-50

@DianaLease
Copy link
Member

sure @sanketshevkar I can handle the merging issue but from the above 3 options which one do you prefer for preview output, I believe the whole window would be a good option as referred below as well.

as discussed on the call today, let's go with the whole preview window, thanks!

@nitro56565 nitro56565 force-pushed the nitro56565/layout-and-UI-fix branch from 87c3c82 to f58fe59 Compare February 27, 2025 07:14
Signed-off-by: nitro56565 <nitro56565@gmail.com>
…tour

Signed-off-by: nitro56565 <nitro56565@gmail.com>
Signed-off-by: nitro56565 <nitro56565@gmail.com>
Signed-off-by: nitro56565 <nitro56565@gmail.com>
Signed-off-by: nitro56565 <nitro56565@gmail.com>
Signed-off-by: nitro56565 <nitro56565@gmail.com>
@nitro56565 nitro56565 force-pushed the nitro56565/layout-and-UI-fix branch from cdde354 to ecefd34 Compare February 27, 2025 13:26
@nitro56565
Copy link
Contributor Author

Hi @sanketshevkar @DianaLease I have fixed the test case as well as the whole preview window for start tour

@DianaLease DianaLease merged commit 75a768a into accordproject:main Mar 5, 2025
7 checks passed
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