-
Notifications
You must be signed in to change notification settings - Fork 13
added user layout interaction support #279
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
base: main
Are you sure you want to change the base?
Conversation
|
We actually just recently created a ticket for this ourselves. So i'm happy you have made an implemention. However it seems overly complex and I think it could be made a lot simpler by using this NPM package: https://www.npmjs.com/package/re-resizable If you want to have a crack at it that would be great. Also please convert back the functions to arrow functions :) |
Was a quick and dirty version from yesterday. Thx for the hint to the re-resizable package. Looks great, but i will need to test if it will break the current full width/hight grid. Will test it on Thursday. Until then feel free to PR your own and will vote which will create cleaner code. |
|
@maxfrederiksen-ess ready for review. Using re-resizable, it produced much cleaner code. ;) |
|
Great! I took at quick look at this and it works a bit better but still far from complete. Here are some points I noticed that are broken:
|
I just tested it again and you are right. Seems like I had the only edge case where it worked fine. Some grouped logs with many replies. Then it covers the whole screen and scrolling is working properly. The MD layout was broken before, therefor I would create a separate PR for it, but I can integrate it as well. |
|
Make sure you have enough logs to go beyond viewport height. When you say MD layout you mean small screens? I noticed this as well that it wasn't working exactly as intended, however, it was still stacking, but yours overlays, that's the major difference. I have just added this fix to main since it was quite small so you can rebase and it should be fine. Improving the responsiveness is not something we have prioritized since majority uses desktop, however it should work as before. If you're unsure how it's supposed to work just compare with latest main. |
Thanks, for the fix in the main. I will rebase and commit the fixes, might take until next week, since other projects came up with higher priority. |
|
Bugs are fixed. Now, small screens viewports seems to work decent enough to scroll through logs if ever needed. |
src/views/SearchView.jsx
Outdated
| })); | ||
|
|
||
| const SearchView = styled(({ className }) => { | ||
| function SearchView() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why did you remove arrow function and styled wrapping?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it looks cleaner.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Gotcha. Well the consensus and standard is using arrow functions so we should revert to that. The styled and classname i see you moved directly into ContentView so thats fine
|
Yeah the config and manual resizing logic seems a bit too much. Im hoping there is a simpler solution but I know how finnicky this kind of feature is so I wouldn't mind taking a look and refining it ones the big things are in place 👍 |


Summary of Changes
Closes #278
Added a Callback function to divider to enable users to change viewport ratio between LogDetailsContainer and LogDetailsContainer
Visual Inspection