-
Notifications
You must be signed in to change notification settings - Fork 116
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
feat(controls): Add react versions of fullscreen and zoom controls #1283
Conversation
@@ -0,0 +1,38 @@ | |||
import React from 'react'; | |||
import fullscreen from '../../../Fullscreen'; | |||
import IconFullscreenIn24 from '../icons/IconFullscreenIn24'; |
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.
Is there any benefit to have these icons in box-ui-elements?
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 don't think so, since they're not part of our standardized icon set. We may refresh them at some point in the future, at which point that may make more sense.
|
||
export default function FullscreenToggle({ onFullscreenToggle }: Props): JSX.Element { | ||
const [isFullscreen, setFullscreen] = React.useState(false); | ||
const Icon = isFullscreen ? IconFullscreenOut24 : IconFullscreenIn24; |
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.
Should these be reversed?
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 don't think so?
|
||
return ( | ||
<button className="bp-FullscreenToggle" onClick={handleClick} title={title} type="button"> | ||
<Icon /> |
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.
Should we introduce Tooltip here in these buttons or will that be a follow on enhancement?
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.
This is meant to replicate the existing experience, so we're still using title
for the text tooltip.
align-items: center; | ||
justify-content: center; | ||
min-width: 48px; | ||
color: #fff; |
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.
Should we use the SCSS variable from box-ui-elements?
scale?: number; | ||
}; | ||
|
||
export const MAX_SCALE = 100; |
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.
Should this be 1
?
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 don't think so? A scale of 1
would equate to a zoom of 100%
, which would be a low maximum.
No description provided.