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

[WIP] Adding hovering and active states to the player buttons #289

Closed
wants to merge 582 commits into from

Conversation

@hibroseph
Copy link
Contributor

commented Mar 14, 2019

Going to keep updating here so you can check on what I am doing and give input as I am coding. I still need to add those styling elements to the forward and backward.

What do you think of the styling currently on the pause button?
#286

nukeop and others added 15 commits Jan 19, 2019
Fixed non-master album info being fetched from the wrong source.
Changed genre from using album.styles to album.genres. Fixed genre not showing up.
Fixed artists without artwork not loading.
@nukeop

This comment has been minimized.

Copy link
Owner

commented on server/main.dev.js in 7560ebe Jan 21, 2019

This should be tested for platform, maximize button should not switch to fullscreen on Windows/Linux, since this isn't the expected behavior.

This comment has been minimized.

Copy link
Collaborator Author

replied Jan 21, 2019

Agreed, changed this in latest commit

trekiteasy and others added 11 commits Jan 21, 2019
Updates the requirements on [react-beautiful-dnd](https://github.com/atlassian/react-beautiful-dnd) to permit the latest version.
- [Release notes](https://github.com/atlassian/react-beautiful-dnd/releases)
- [Changelog](https://github.com/atlassian/react-beautiful-dnd/blob/master/CHANGELOG.md)
- [Commits](https://github.com/atlassian/react-beautiful-dnd/commits/v10.0.3)

Signed-off-by: dependabot[bot] <support@dependabot.com>
…eact-beautiful-dnd-tw-10.0.3

Update react-beautiful-dnd requirement from ^9.0.0 to ^10.0.3
Capitalized some text
Caps
@CAnderson97

This comment has been minimized.

Copy link
Contributor

commented on 0646a09 Jan 22, 2019

This commit broke adding songs to the queue for me. I get the error "Invariant failed: Draggable requires draggableId"

To get this error I had to re-run "npm install"

This comment has been minimized.

Copy link
Owner

replied Jan 22, 2019

Ok, I'll revert this.

@CAnderson97

This comment has been minimized.

Copy link
Contributor

commented on 9d59abf Jan 22, 2019

I think this commit broke adding playlists to the queue.

Open playlist view
Select a playlist
Click Play
Nothing Happens

This comment has been minimized.

Copy link
Collaborator Author

replied Jan 22, 2019

Correct. Fixed in df11299

hibroseph and others added 8 commits Mar 12, 2019
…se logical operation
Disable back and forward buttons when no history exists
@nukeop

This comment has been minimized.

Copy link
Owner

commented Mar 14, 2019

To keep things consistent, it's better to use colors from the palette (vars.scss), instead of directly setting colors.

@hibroseph

This comment has been minimized.

Copy link
Contributor Author

commented Mar 14, 2019

@nukeop

This comment has been minimized.

Copy link
Owner

commented Mar 15, 2019

To be clear, feel free to use scss functions like darken or rgba (to set opacity) with palette colors, like you're doing now.

hibroseph added 2 commits Mar 15, 2019
@hibroseph

This comment has been minimized.

Copy link
Contributor Author

commented Mar 15, 2019

I just want to say, it took me WAY too long to realize that the scss operators (like the ampersand & or the color) need to have spaces. A lack of spaces between will alter their operation.

@hibroseph

This comment has been minimized.

Copy link
Contributor Author

commented Mar 15, 2019

How do they look right now? Should I do an active state?

hibroseph added 2 commits Mar 15, 2019
…on time duration from palatte
@nukeop

This comment has been minimized.

Copy link
Owner

commented Mar 16, 2019

You don't need to use & if you use a space.

.a {
  .b { }
}

Is identical to

.a .b { }

Which in both cases selects all bs inside as. However,

.a {
  &.b {

  }
}

selects everything that has both class a and b at the same time.

@hibroseph

This comment has been minimized.

Copy link
Contributor Author

commented Mar 16, 2019

What about with puesdos? Don't I need the ampersand in front of puesdos, such as

&:not(.player_button_disabled) :hover {
    transition: $short-duration; 
    color: darken($white, 50%);
  }
@nukeop

This comment has been minimized.

Copy link
Owner

commented Mar 16, 2019

Yes, the ampersand is a shorthand for referring to the selector from the level above, so e.g.

.a {
 &:hover { }
}

Refers to a:hover.

The selector you created in your comment refers to any hovered-over element that is a child of an element that does not have the class .player_button_disabled (depending on how this is nested).

However,

.a {
  :hover {}
}

selects children of .a that are hovered over.

@hibroseph

This comment has been minimized.

Copy link
Contributor Author

commented Mar 16, 2019

@nukeop

This comment has been minimized.

Copy link
Owner

commented Mar 16, 2019

As long as it works as you intended, it's all good.

@hibroseph

This comment has been minimized.

Copy link
Contributor Author

commented Mar 26, 2019

@nukeop sorry I didn't let you know what I had done. Want to take a look at it now?

@nukeop

This comment has been minimized.

Copy link
Owner

commented Mar 26, 2019

Sure, I'll review soon after you commit your changes, no rush.

@nukeop

This comment has been minimized.

Copy link
Owner

commented Jun 17, 2019

Hi, are you still working on it, or can it be improved/finished by somebody else? Or would you rather have us not use your code?

@hibroseph

This comment has been minimized.

Copy link
Contributor Author

commented Jun 19, 2019

Sorry about that! I thought I had committed my changes. Let me see where I left it at and commit those changes (if I have any)

@nukeop

This comment has been minimized.

Copy link
Owner

commented Jun 20, 2019

Ok, cool.

@nukeop nukeop force-pushed the nukeop:master branch from 13f1d88 to 81de3d1 Sep 30, 2019
@nukeop nukeop closed this Oct 3, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
You can’t perform that action at this time.