[WIP] Left-Sidebar updated #64
Conversation
|
A general observation, please don't work on your master/main branch. It's much better to create feature branches. |
|
Will take care of making branches from the next time. I am still facing the same issue after passing |
|
I've pushed something to your branch, that seems to be a bit closer to solving this. |
|
It leads to the same type annotation problem because |
|
LeftSidebar completed and dropdown menu has been added to RightSidebar. Functionality is yet to be added to the RightSidebar. LeftSidebar RightSidebar dropdown menu |
package-lock.json
Outdated
| @@ -1,9868 +1,8 @@ | |||
| { | |||
| "name": "elm-chorus", | |||
| "version": "1.0.0", | |||
| "lockfileVersion": 2, | |||
| "lockfileVersion": 1, | |||
There was a problem hiding this comment.
You might want to use a newer version of npm (version 14 at least) as we would like to use the newer version of the package lock
src/Components/RightSidebar.elm
Outdated
| @@ -45,3 +46,21 @@ view rightSidebarExtended rightSidebarMsg panelHeight connection = | |||
| [ height (px panelHeight), width (px 50), Background.color Colors.playlistBackground, alignRight, htmlAttribute <| Html.Attributes.style "pointer-events" "all" ] | |||
| [ Input.button [ centerX, height (px 50) ] { onPress = Just rightSidebarMsg, label = Element.text "<" } | |||
| ] | |||
|
|
|||
| rightSidebarMenuDropDown : Bool -> List (Attribute msg) | |||
| rightSidebarMenuDropDown rightSidebarMenu = | |||
There was a problem hiding this comment.
I think it would be good to name rightSidebarMenu something along the lines of isRightSidebarMenuVisible or showRightSidebarMenu. As it was very unclear to me, what this variable was about, when I saw it in the other places that the name is used.
src/Shared.elm
Outdated
| @@ -50,6 +50,16 @@ type alias Model = | |||
| , connection : Connection | |||
| , rightSidebarExtended : Bool | |||
| , controlMenu : Bool | |||
| , musicHover : Bool | |||
There was a problem hiding this comment.
While this works, I would recommend to have a custom type that represents what is hovered. As there is only ever one thing that can be hovered in this case.
This has a lot more possible states, that might happen and are unwanted. Basically all states that are have more then one true.
It would be nicer with type LeftSidebarMenuHover = None | Music | Movies | ...
There was a problem hiding this comment.
I have checked the model and application, at a time only one icon can be hovered and no other states are possible. Do I still need to make any changes to the model?
There was a problem hiding this comment.
I would recommend it, as it vastly improves the design of your store and thus the handling. Also, while you think it's not possible, eliminating it at the source will make it impossible to happen.
There was a problem hiding this comment.
Yeah I think you are right. I will start working on it straight away.
|
In general this looks okay, there are a lot of places where we can possibly shorten the code. But I think we should start slow, so this is fine to me. Please have a look at the other comments, before this can be merged. |
|
Okay will try to solve all of them one by one. |
|
@razzeee , please have a look. |
|
LGTM, thank you |
Hey @razzeee, I am working on Issue #34 and trying to add onMouseHover to all the icons in the left sidebar. There are some errors due to return type of
viewand I am facing problem fixing it. Can you can help me fix this or suggest a better way to do this .