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

panels restructure #901

Closed
3 tasks
edenvidal opened this issue Jan 26, 2025 · 50 comments
Closed
3 tasks

panels restructure #901

edenvidal opened this issue Jan 26, 2025 · 50 comments
Assignees

Comments

@edenvidal
Copy link
Member

  • move the code-view right (switch design/code views), as in the design.
    Image
  • have the code view overlaying the design instead of occupying its area.
    • have the code views' background semitransparent and blurring its underneath content (example)
@edenvidal edenvidal converted this from a draft issue Jan 26, 2025
@edenvidal
Copy link
Member Author

@kareem-g?

@atulbhatt-system32 atulbhatt-system32 moved this from Next Up to In Progress in rnbw 0.1 Feb 2, 2025
@edenvidal edenvidal moved this from In Progress to Next Up in rnbw 0.1 Feb 3, 2025
@edenvidal edenvidal moved this from Next Up to In Progress in rnbw 0.1 Feb 3, 2025
@atulbhatt-system32
Copy link
Contributor

atulbhatt-system32 commented Feb 4, 2025

@edenvidal
in this example for semi transparent background we must always have a constant background color. Here I tried installing the zoo app and haven't found the option to change background from the shade of gray to any other that's why the glass-morphism effect works on it. It won't be same for us.

Because in our case the background of the design view keeps on changing.

Now how do you want to me handle this. And if we can't have semi-transparent effect does it makes sense to have overlaying codeview?

Below is sample for same effect different background for stage.

For now overlay codeview is not completed as would also like to know if it can be resized and if yes then upto what extent.

Image Image

@edenvidal
Copy link
Member Author

@atulbhatt-system32 so no worries about the semi transparent effect 🙌
let's make it resizable to whatever you feel and i'll provide feedback for it as soon as its on dev

@atulbhatt-system32
Copy link
Contributor

@edenvidal so will it still be overlay or just position should be changed. Because overlay without semi-transparent won't make sense.

@edenvidal
Copy link
Member Author

@atulbhatt-system32 got it. so check it out

Kapture.2025-02-04.at.16.52.24.mp4

@atulbhatt-system32 atulbhatt-system32 moved this from In Progress to In Review in rnbw 0.1 Feb 5, 2025
@atulbhatt-system32
Copy link
Contributor

@edenvidal I pushed this change too to the dev you can check an give me the feedback.

@atulbhatt-system32
Copy link
Contributor

@edenvidal updated dev with following changes:

  • Codeview peek on hover without affecting the other panels
  • Action Panel Width is now back to how it was before
  • Actions button in the actions panel can overlap the text if the width of action panel is not enough
  • Fixed: resizing action panel affecting codeview size

@edenvidal
Copy link
Member Author

@atulbhatt-system32 amazing! the only thing i'm discovering now is that in preview mode, the code view loses the two-way syncing and highlighting experience.

steps to reproduce:

  1. click on different elements in the sidebar, design, and code panels.
  2. switch between different panel states.
  3. observe when the context is lost.

@atulbhatt-system32
Copy link
Contributor

@edenvidal I understood the issue. However, this has to be covered separately as it's related to monaco. There are 2 separate instances of codeview that are getting created. And this might be the reason for the preview mode not able to register the context.

The monaco setup we have was not intended for multiple instances. This has to be covered in separate milestone. Probably in issue #892

@edenvidal
Copy link
Member Author

@atulbhatt-system32 got it. cool. can we simplify that behavior more? i see those panel states as purely visual, handled with css. but in the code, the logic is too tightly connected to things it doesn’t need to be. see my point? if the panels are always there and always work the same way, why would they break just because of different visual conditions?

@atulbhatt-system32
Copy link
Contributor

@edenvidal are you here pointing out the logic we used to handle the panel behaviour during resize?

The panels don't break but as I mentioned before when the resolution changes the width of the panel changes and the area covered by panels is based on the percentage of the width of the window.

But our content inside the panels are in px.

When the panels loads it uses the fix percentage of width to cover. Now to solve this issue what I have done is to calcualte what percentage of the width will equal to the px we need.

And I checked for other libraries for it but turns out this one is the best out there. Even shadcn resize component uses it.

@edenvidal
Copy link
Member Author

@atulbhatt-system32 this isn't what i meant. the ux now is fine. i was in the same context as before. break = out of sync. see my point?

@atulbhatt-system32
Copy link
Contributor

The preview modes are not part of panel.

@edenvidal
Copy link
Member Author

edenvidal commented Feb 6, 2025

@atulbhatt-system32 i am confused. let's start our conversation back from here - #901 (comment). and anyway as you mentioned it is ok to complete it within the sprint of this other issue. (but do please gain the right context around my questions)

@atulbhatt-system32
Copy link
Contributor

atulbhatt-system32 commented Feb 6, 2025

@atulbhatt-system32 got it. cool. can we simplify that behavior more? i see those panel states as purely visual, handled with css. but in the code, the logic is too tightly connected to things it doesn’t need to be. see my point? if the panels are always there and always work the same way, why would they break just because of different visual conditions?

The things that are put inside the panel have there own behaviour. They are not connected to panels it's their own behaviour. Even if panel didn't exist and we were to put 2 codeviews the chances of facing the same error exists.

I hope I'm addressing the right thing

@edenvidal
Copy link
Member Author

edenvidal commented Feb 6, 2025

still not @atulbhatt-system32. my question is simply why the panels are not open all the time, but when we hit "esc" we simply change css properties. there's no "state" this is just pure css. see my point?
this is how we can ensure it never goes out of sync. (re-"out of sync", are we on the right page for the what i consider as bug?)
thanks.

@atulbhatt-system32
Copy link
Contributor

atulbhatt-system32 commented Feb 6, 2025

@edenvidal I understood.

Okay so we need to eliminate use of state and do the hiding and showing of panel using css. Yeah we can try that.

We change the css property using js.

Am I right @edenvidal ?

@edenvidal
Copy link
Member Author

We can get back to it after getting some progress with the notifications if you like ❤️

@edenvidal
Copy link
Member Author

and who knows maybe it can be a state but it only affect CSS. I think we should aim for it as much as possible.

@edenvidal
Copy link
Member Author

Think about the two ways, synchronization is something holy and isolated that cannot be broken. It makes sense to me that the sidebar, design, and code, are always there all the time. But the panel showing up or not will be just a matter of visibility (CSS)

@atulbhatt-system32
Copy link
Contributor

@edenvidal I would be starting notification tomorrow. But yeah I got the point here. I also want to minimize the state triggers so will try to follow this approach.

@edenvidal
Copy link
Member Author

thank you. i am happy to achieve this.

@edenvidal
Copy link
Member Author

edenvidal commented Feb 9, 2025

@atulbhatt-system32 when the design is in preview mode, performing actions does nothing (it was working previously). the mode is broken, which further justifies detaching it from presentational-specific aspects.
try doing things on this page - https://rnbw.dev/#/weareunder.design/brandsprint/index.html

@atulbhatt-system32
Copy link
Contributor

I am working on the same. Already tried not doing conditional render for codeview and showing and hiding panels via panel resizing function, which basically hides it using css but it's not working.

This is more of a monaco thing where having 2 different instances with same content is not syncing which I'm trying to achieve. I will fix this as this is the problem for many related things.

@atulbhatt-system32
Copy link
Contributor

@edenvidal pushed some changes on dev.

Action should work now and two way syncing should also work now for preview.

@edenvidal
Copy link
Member Author

@atulbhatt-system32 please describe what is done, what is fixed, what to expect...

@atulbhatt-system32
Copy link
Contributor

atulbhatt-system32 commented Feb 9, 2025

@edenvidal

when the design is in preview mode, performing actions does nothing

This should work now

2 way syncing should work from preview mode

@edenvidal
Copy link
Member Author

@atulbhatt-system32 yeah, but can you explain how you fixed it? can you give more details? what exactly was done, what was fixed, and what should we expect?

@edenvidal
Copy link
Member Author

hey @atulbhatt-system32, it seems like the issue is still there. the code view still doesn't highlight properly—it feels like there are two different code views. not sure what's causing it. why in the world do we have to get the sidebar+code out of the dom. ever.
also, the reason i'm asking about matching expectations is that i don't really know to what extent something has been fixed. as a client, that uncertainty can be quite frustrating. definitely, there’s a delta between what i perceive as a good user experience and the current outcome

@atulbhatt-system32
Copy link
Contributor

@edenvidal for now I removed the conditional rendering of panel and used panel resizing mechanism which solved the action not running after hiding codeview.

For preview mode syncing I tried a lot of things like using monaco model but it didn't helped as I also wanted to retain the scroll position of panel codeview and preview mode but the sync wasn't happening.

Then I started tracking every actions inside codeview to debug why the node selection is not happening and then i found we weren't getting the selection value for the nodes.

Before we trying to get the selection value from monacoEditorRef but it wasn't giving us that. So I got that value directly from editor instead of editorRef which was changing during renders.

@edenvidal
Copy link
Member Author

@atulbhatt-system32, even after preview mode is on and loses sync, the code no longer highlights with the design. it loses context. it loses the one principle it’s all about.

@atulbhatt-system32
Copy link
Contributor

@edenvidal nothing is out of dom.
<Codeview/> is a component.

<Panel> lies inside <PanelGroup/> to achieve the resizing behaviour.

Now the Codeview Preview is an overlay so it needs to be at top layer of panels we can't have it inside the panels.

So we have <Codeview/> component inside the <PanelGroup> and then outside it.

@atulbhatt-system32
Copy link
Contributor

atulbhatt-system32 commented Feb 9, 2025

@edenvidal yeah I see currently only clicking on code is syncing to other views for preview mode. But not vice-versa.

@edenvidal
Copy link
Member Author

edenvidal commented Feb 9, 2025

@atulbhatt-system32 wdyt about...

  • remove the panel resizing components completely
  • keep only sidebar, design, and code views. always there! (:
  • sidebar and code are stacked by default, (display: hidden) in preview mode independently as usual.
  • no need for resizing logic or dependencies—handled via css instead (i will take care of making 100% resizable panels with css as should be. cool?)

@atulbhatt-system32
Copy link
Contributor

@edenvidal I will update you on this tomorrow.

@atulbhatt-system32
Copy link
Contributor

@edenvidal I pushed changes to dev again.

After recognising the last solution and thinking about it I got an idea on how to solve it.

So, having 2 editor instance were creating 2 different editor ref but we were storing only 1 global ref which was getting overwritten by the last most editor that was mounting.

So now I created separate global instances of editor based on their unique instance id. This solution seems to work. I have tried some basic testing with it.

Will test it more tomorrow. But now the syncing seems to work for all cases.

It would be great if you can also give it a try as in my mind there are patterns to test which creates a blindspot for me to find the errors or missing cases.

@edenvidal
Copy link
Member Author

thanks @atulbhatt-system32, but that sounds so inefficient. what if we just go with my suggested solution? it makes so much more sense, avoids future problems, and improves performance. please, let's do it this way.

@atulbhatt-system32
Copy link
Contributor

atulbhatt-system32 commented Feb 9, 2025

@edenvidal sure I will do it the way you asked.

But I need to know that initially the sidebar and codeview are stacked and not overlaying design view.

Sidebar | Design View | Codeview

But when sidebar or codeview are hidden they will be overlaying designview?

Is it still the same thing but without resizable panels?

@edenvidal
Copy link
Member Author

edenvidal commented Feb 9, 2025

exactly. when hidden, they are also absolute. the "design" has no defintion. it also pushed itself to be 100%. it is only the sidebar/code being dynamic.
resizing will be achievable through css.
and with this we fix 2+ original sins.
@atulbhatt-system32

@atulbhatt-system32
Copy link
Contributor

@edenvidal I pushed the changes for this to dev.

Haven't removed the library though as it's being used by sidebar for workspace view and html node view.

I haven't implemented resizing using css.

@atulbhatt-system32
Copy link
Contributor

@edenvidal I also pushed another branch where I just now achieved same with only one codeview instance with Resizable panels.

resizable-preview is the branch name.

@edenvidal
Copy link
Member Author

thanks @atulbhatt-system32. in the recent branch, the code view isn't resizing properly.
but my bigger question is—did we achieve having just one sidebar, one design view, and one code view? if so, let's keep it. or is the css version (dev) better in terms of stability and separation of concerns (presentation vs. functionality)? if so, maybe we should keep that one instead.
and if we go with the css version, you can remove the resizable component entirely—i'll handle resizing later with css modules.

wdyt?

@edenvidal
Copy link
Member Author

@atulbhatt-system32 update: after testing the dev version, I’m convinced it’s much better and faster. let’s go with it. ditch the resizable panels completely and remove any redundant layers or functionality that was added due to the separation of modes.

@atulbhatt-system32
Copy link
Contributor

@edenvidal The panels are not using the concept of resizable as a group.

However, the Sidebar or ActionPanel is independently using the library for the panel within it.
Image

@edenvidal
Copy link
Member Author

@atulbhatt-system32 not sure i understand. let's focus and sum it up:

  • confirm this is solely handled by css now and there is always only one sidebar, one design, and one code
  • remove the resizable panels and resizable functionality completely, even it it exists inside panels. i don't want it and will handle this differently. remove the dependency and everything.
  • delete all redundant branches.
    thanks.

@atulbhatt-system32
Copy link
Contributor

@edenvidal code is pushed to dev.

  • Sidebar is now not resizable for fileview and nodetree view. And there are always one sidebar, one design and one codeview.

  • the library is removed now

  • now extra branches exist

@edenvidal
Copy link
Member Author

thanks, @atulbhatt-system32!

the only issue now is that the "Esc" key doesn't work when there's activity in the sidebar or code panel. If either is active, pressing "Esc" has no effect. Try hitting "Esc" after clicking on the code panel or sidebar to see the issue.

@atulbhatt-system32
Copy link
Contributor

@edenvidal

can you give me step by step process to produce this.

I select codeview and press eacape the Codeview remains visible for me as it is currently in preview state as soon as it goes out of stacked state. But the sidebar for me hides. And as soon as I move my mouse out of codeview focus it also hides.

And same goes when I do it with focus on Sidebar while toggling with esc.

A video recording for this will help.

@edenvidal
Copy link
Member Author

hey @atulbhatt-system32, i think i got it. it's actually really smart and makes sense now. when you hover over the sidebar and press esc, it doesn't hide immediately, but it does enter the "hidden" state. however, it won't apply to the specific panel you're hovering over at that exact moment. pretty clever! i think this issue is done.

@github-project-automation github-project-automation bot moved this from In Review to Done in rnbw 0.1 Feb 11, 2025
@atulbhatt-system32
Copy link
Contributor

@edenvidal also pushed to dev again as there were some layout issue when selecting a file like js which doesn't renders nodeview

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

No branches or pull requests

2 participants