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

Fullscreen height #112

Merged
merged 20 commits into from
Mar 27, 2024
Merged

Fullscreen height #112

merged 20 commits into from
Mar 27, 2024

Conversation

LukaszMMazur
Copy link
Contributor

No description provided.

@LukaszMMazur LukaszMMazur requested a review from a team as a code owner March 19, 2024 18:51
@mazurroman mazurroman linked an issue Mar 19, 2024 that may be closed by this pull request
Copy link
Contributor

@mazurroman mazurroman left a comment

Choose a reason for hiding this comment

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

Hey @LukaszMMazur thank you for your contribution! There are a few improvements we would like to see before accepting this PR. Namely:

  • The app, when in fullscreen, doesn't expand all the way to the bottom. See screenshot attached from my screen.
    CleanShot 2024-03-20 at 08 54 50@2x

  • Fullscreen mode should temporarily hide the top banner. Currently, the banner stays visible which looks broken. See screenshot:
    CleanShot 2024-03-20 at 08 56 18@2x

  • When I click "Compile and run", the app expands vertically. What we want instead is to keep the height constant so that there are no jumps. It would be great to fix that as part of this issue. See video attached.

CleanShot.2024-03-20.at.09.01.48.mp4

Copy link
Contributor

@mazurroman mazurroman left a comment

Choose a reason for hiding this comment

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

It's starting to look really really good, great job @LukaszMMazur !!

I added some minor comments into the code. On top of that, here are some improvements we can do to make it really shine:

In non full screen mode only:

  • Make the app a bit taller by default, i.e. by 20-30%
  • Set static height for the dark section in the bottom, so that it doesn't jump when the "Compiler version" text appears

In full screen mode only:

  • Replace "Cairo VM Playground" with the cairovm.codes logo
  • Copy "fullscreen", "dark mode" and "cmd + k" buttons into the dark info bar at the bottom of the screen (the same one that holds "compiler version"), so that the buttons are available also in full screen. Position the buttons to the right of the info bar
  • Add text "Made with 🖤 by Walnut" to the dark info bar at the bottom of the screen, next to the "fullscreen" etc buttons. Walnut should be a link that opens walnut.dev
  • Expand the app 100% width and height, no need to have extra white margin around the app when in full screen. Careful about the rounded border corners - they should not be rounded when in full screen so that the app fills the full space

@@ -23,7 +27,11 @@ const AboutCairoVMBanner = () => {
}

return (
<div className="relative bg-gray-50 dark:bg-black-700 pb-6 mt-0 mb-10">
<div
Copy link
Contributor

Choose a reason for hiding this comment

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

Better not to render the component at all in case its full screen, instead of using hidden class.

See the "Inline If with Logical && Operator" section here: https://legacy.reactjs.org/docs/conditional-rendering.html#inline-if-with-logical--operator

So it will look like:

!isFullscreen && (
  <div className ... ></div>
)

<div
className={`flex flex-col md:flex-row ${
isFullScreen ? 'h-[92vh]' : ''
}`}
Copy link
Contributor

Choose a reason for hiding this comment

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

can you use the cn object for assigning conditional classes (import cn from 'classnames')

@mazurroman mazurroman marked this pull request as draft March 22, 2024 15:42
@LukaszMMazur LukaszMMazur marked this pull request as ready for review March 22, 2024 15:51
@LukaszMMazur LukaszMMazur marked this pull request as draft March 22, 2024 19:03
@LukaszMMazur LukaszMMazur marked this pull request as ready for review March 23, 2024 08:02
@LukaszMMazur LukaszMMazur marked this pull request as draft March 24, 2024 15:43
@LukaszMMazur LukaszMMazur marked this pull request as ready for review March 24, 2024 18:04
@mazurroman mazurroman merged commit 6ac51cc into walnuthq:main Mar 27, 2024
2 checks passed
@LukaszMMazur LukaszMMazur deleted the Fullscreen branch April 23, 2024 18:22
@LukaszMMazur LukaszMMazur restored the Fullscreen branch April 23, 2024 18:22
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.

feat: Fullscreen
3 participants