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

Add fullscreen when viewing the app info #367

Merged
merged 12 commits into from Apr 15, 2022
Merged

Add fullscreen when viewing the app info #367

merged 12 commits into from Apr 15, 2022

Conversation

fmterrorf
Copy link
Contributor

closes #366

Peek.2022-04-12.21-34.mp4

Copy link
Member

@mcrumm mcrumm left a comment

Choose a reason for hiding this comment

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

Wow thanks @fmterrorf this looks great! Can you please roll back the dist changes tho- for security reasons we won't accept a PR that includes generated assets. Thanks!

@fmterrorf
Copy link
Contributor Author

@mcrumm I've reverted the changes in dist. I noticed that the package-lock also got changed. Should I revert that as well?

@fmterrorf fmterrorf requested a review from mcrumm April 13, 2022 04:31
@mcrumm
Copy link
Member

mcrumm commented Apr 13, 2022

@fmterrorf Yes, please do. If it needs to be updated we'll be sure to do so when we bump the build :)

@josevalim
Copy link
Member

This is awesome!

I also think we can fully implement this using Phoenix.LiveView.JS and avoid the additional server state. Can you please check/try?

Co-authored-by: José Valim <jose.valim@gmail.com>
@fmterrorf
Copy link
Contributor Author

I can definitely just use JS remove/add_class! I'll have a look more tomorrow

@fmterrorf
Copy link
Contributor Author

fmterrorf commented Apr 14, 2022

Updated it to use JS commands. It looks like this right now:

Peek.2022-04-13.19-31.mp4

@josevalim
Copy link
Member

It looks great! One last detail. The new full screen button is shifting the modal header so the title is no longer vertically in the center. Can you please take a look? Here is how it looks today:

Screenshot 2022-04-14 at 07 58 11

@fmterrorf
Copy link
Contributor Author

@josevalim Thanks for pointing that out! The new commits should line it up properly now

Master

Screenshot from 2022-04-14 09-15-59

This branch
Screenshot from 2022-04-14 09-23-31

@fmterrorf
Copy link
Contributor Author

I'll fix up the test in a few 👀

@josevalim
Copy link
Member

@fmterrorf also please don't commit changes to dist, I will do so after nmerging!

@fmterrorf
Copy link
Contributor Author

Test should be good now. I ran it locally and only got the 1 failure from test/phoenix/live_dashboard/pages/ecto_stats_test.exs:110 because i dont have a mysql container running

@josevalim josevalim merged commit ceae17b into phoenixframework:master Apr 15, 2022
@josevalim
Copy link
Member

💚 💙 💜 💛 ❤️

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.

Add an option to view the App info modal fullscree
3 participants