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

feat(controls): Add react versions of fullscreen and zoom controls #1283

Merged
merged 1 commit into from
Nov 3, 2020

Conversation

jstoffan
Copy link
Collaborator

@jstoffan jstoffan commented Nov 3, 2020

No description provided.

@jstoffan jstoffan requested a review from a team as a code owner November 3, 2020 23:03
@@ -0,0 +1,38 @@
import React from 'react';
import fullscreen from '../../../Fullscreen';
import IconFullscreenIn24 from '../icons/IconFullscreenIn24';
Copy link
Contributor

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?

Copy link
Collaborator Author

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;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should these be reversed?

Copy link
Collaborator Author

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 />
Copy link
Contributor

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?

Copy link
Collaborator Author

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;
Copy link
Contributor

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;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be 1?

Copy link
Collaborator Author

@jstoffan jstoffan Nov 3, 2020

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.

@jstoffan jstoffan merged commit 443746e into box:master Nov 3, 2020
@jstoffan jstoffan deleted the react-ui-fullscreen-zoom branch November 3, 2020 23:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants