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

Remove position computations, use MUI components and new splitter #135

Merged
merged 6 commits into from
Jul 23, 2024

Conversation

WardBrian
Copy link
Collaborator

I initially intended only to address #71 (comment), but maintaining our current layout quickly led me to use some more MUI components and remove the absolute positioning. Apologies for the monolithic PR

My goal was not to change any layouts or functionality, though some visual features have changed, mostly due to using MUI more
Screenshot from 2024-07-19 16-15-21

Copy link
Collaborator

@jsoules jsoules left a comment

Choose a reason for hiding this comment

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

Couple possible tweaks. Overall I think it looks good, but will want to take a deeper look on Monday.

gui/src/app/FileEditor/StanFileEditor.tsx Outdated Show resolved Hide resolved
gui/src/app/FileEditor/StanFileEditor.tsx Outdated Show resolved Hide resolved
gui/src/app/FileEditor/TextEditor.tsx Outdated Show resolved Hide resolved
gui/src/app/MainWindow.tsx Outdated Show resolved Hide resolved
gui/src/app/components/TabWidget.tsx Outdated Show resolved Hide resolved
gui/src/app/components/Splitter.tsx Show resolved Hide resolved
gui/src/app/pages/HomePage/HomePage.tsx Outdated Show resolved Hide resolved
gui/src/app/pages/HomePage/HomePage.tsx Show resolved Hide resolved
gui/src/app/pages/HomePage/LeftPanel.tsx Outdated Show resolved Hide resolved
@magland
Copy link
Collaborator

magland commented Jul 20, 2024

Overall this looks good to me. I think the top bar takes up too much vertical space now.

This causes challenging conflicts with #127 (and maybe #132 as well). I started to look at it but it wasn't going smoothly. How do you propose we deal with that? Since those involve tab widgets, I think it would have been better if we had merged those first. Do you think it's reasonable to merge those and then apply the changes of this PR? Or other way around?

@WardBrian
Copy link
Collaborator Author

Yeah, I realized as I was going it would cause some conflicts going either way. I am willing to do the merge resolution on whichever one doesn’t go first, but I don’t have a strong opinion on the ordering there

@magland
Copy link
Collaborator

magland commented Jul 20, 2024

I am willing to do the merge resolution on whichever one doesn’t go first

That would be great if you are willing to do that, because I was getting confused in my attempt. :)

@magland
Copy link
Collaborator

magland commented Jul 22, 2024

I tested this out and I think the changes all look good, except IMO the title bar has too much vertical space.

@WardBrian
Copy link
Collaborator Author

@magland -- I've updated the top bar to use MUI's dense variant, which trimmed a lot of the padding:

Before:
image

After:
image

@WardBrian WardBrian requested a review from jsoules July 22, 2024 16:37
@magland
Copy link
Collaborator

magland commented Jul 22, 2024

Okay I think it's still too big, but it's not a huge deal and I wouldn't want to block the merge.

Copy link
Collaborator

@jsoules jsoules left a comment

Choose a reason for hiding this comment

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

I don't think I have anything further on this, if you're happy with it.

@WardBrian WardBrian merged commit 5efe316 into main Jul 23, 2024
2 checks passed
@WardBrian WardBrian deleted the refactor/mui-non-absolute branch July 23, 2024 14:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants