FloatingActionButton: added the component to be used in smaller views#2347
FloatingActionButton: added the component to be used in smaller views#2347lindapaiste merged 8 commits intoprocessing:developfrom
Conversation
dewanshDT
changed the title
| return ( | ||
| <Button | ||
| data-behaviour={isPlaying ? 'stop' : 'run'} | ||
| style={{ paddingLeft: isPlaying ? 0 : remSize(5) }} |
There was a problem hiding this comment.
I don't love this but I know it's here to fix a genuine problem so it's best to keep it for now so as not to fall deep into an SVG rabbit hole. We need this because the PlayIcon does not appear visually centered within its viewbox. IMO that's a problem with the icon and we may want to modify the icon to have whitespace on the side so that it looks centered.
There was a problem hiding this comment.
yeah exactly that's what I thought, but let's keep it for now we'll remove it when we decide to replace the Icon with a better one.
| z-index: 3; | ||
| cursor: pointer; | ||
| ${prop('Button.secondary.default')}; | ||
| aspect-ratio: 1/1; |
There was a problem hiding this comment.
This property has way more support than I thought! It's not really necessary since you've defined both a width and a height but it's not hurting anything so 🤷 .
There was a problem hiding this comment.
Just want to note some of the ideas that we've tossed around regarding this element, potentially for future refactorings but not for right now:
- We could export the
styledpart as a reusableFloatingActionButtonUI component, and make this specificStartStopButtonbe an instance of that commonFloatingActionButton. (This fits with best practices, but is kind of silly since there are no other floating action buttons anywhere else in the app.) - We could extract the existing
StartStopButtonfrom the desktop toolbar into its own component (see Break upToolbarelements #2239). That component would need to take aclassNameprop in order to be stylable withstyled-components. This floating version for mobile could be astyled(StartStopButton), where we only need to specify the styles to override like the position and size. The positioning could also potentially be handled by a separatedivwhich has the button as a child.
There was a problem hiding this comment.
Yeah, the StartStopButton is a great idea and since we do not have any more floating action buttons anywhere else in the app but when I made it I thought it would be reusable and we should definitely export the styled part as a floating action button and the start-stop button should be passed in as a child element. let me know when we want to do it
|
hey @lindapaiste, made the suggested changes, check it out! |
| align-items: center; | ||
| box-shadow: rgba(0, 0, 0, 0.24) 0px 3px 8px; | ||
| &.stop { | ||
| ${prop('Button.primary.default')} |
There was a problem hiding this comment.
we've got a color here with no property!
| width: ${remSize(60)}; | ||
| z-index: 3; | ||
| padding: 0; | ||
| ${prop('Button.secondary.default')}; |
There was a problem hiding this comment.
Another variable that's not assigned to any property.
lindapaiste
left a comment
lindapaiste
left a comment
There was a problem hiding this comment.
Looking good!
Two small changes and then let's merge this:
- Delete the two lines I noted which have a
${prop(that isn't assigned to any property. - I know that hover isn't that important on small-screen but let's put a more obvious hover effect on the "stop" button for when it's on a small laptop or whatever. I'm thinking we change the background to pink. Might be as simple as changing
&.stopto&.stop:not(:hover).
3 participants
This is the part of the mobile responsive project
Changes:
Floating action button component is added, to be used in the mobile views
I have verified that this pull request:
npm run lint)npm run test)developbranch.