-
Notifications
You must be signed in to change notification settings - Fork 104
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
Height-graph width reduced #149
Conversation
@MihirKp25 could you please resolve existing conflicts so we can have a look at your PR? |
I have resolved conflicts. Sorry for inconvenience. |
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.
The whitespace-only changes seem irrelevant, Iβd actually expect prettier to complain.. Can you revert those?
I have made your suggested changes @nilsnolde |
@@ -139,7 +139,6 @@ class Waypoint extends React.Component { | |||
</Label> | |||
} | |||
/> | |||
|
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.
please check your PR again for unnecessary, whitespace-only changes.
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.
Sorry I misinterpreted the last change request and added spaces in this file. I removed those changes. Please let me know if there are any other unnecessary changes I have made. And sorry for the inconvenience.
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.
Fine for me, thanks. I'll let @chrstnbwnkl merge if he's ok with this.
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.
In order for the graph to resize based on the panel's collapsed state, this should not only execute in componentDidMount
(which only runs once), but whenever the side panel's collapsed state changes.
I will try to make your suggested changes asap. |
osm.-.Made.with.Clipchamp_1680012651071.mp4@chrstnbwnkl I have made changes in the function such that the width of the height graph container reduces as the button is clicked. But it seems svg does not have automatic fit content like HTML tags. Can you help me with this error? I have actually used the query selector method to change the width of the container. |
@MihirKp25 can you push your latest changes? The functionality in the video looks pretty close to what we want to achieve, right? |
Changes are pushed @chrstnbwnkl . Also, I want to contact you for review of my GSOC proposal. Can you tell me how can I do this |
This looks pretty good, thanks @MihirKp25 ! You can find more info on the proposal here: https://wiki.openstreetmap.org/wiki/Google_Summer_of_Code/2023 and here: #67. If you follow those guidelines you should be good. We have (un)fortunately too many applicants to do extensive proposal review. I suggest you upload it to GSoC, we'll surely read and consider it. |
π οΈ Fixes Issue
Closes #136
π¨βπ» Changes proposed
I have reduced the width of height graph from 0.9 to 0.75 in Map.jsx file. I tried to change this width dynamically but it didn't end well. But still it looks pretty nice to me and now we can easily see y axis of height graph.
π Note to reviewers
If you find a way of reducing the width dynamically whenever the direction tab closes please tell me :)
π· Screenshots