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

feature(ImageViewer): Add FilterControls component to ImageViewer #402

Merged
merged 13 commits into from
Nov 24, 2020

Conversation

mheitman
Copy link
Collaborator

@mheitman mheitman commented Oct 9, 2020

to: @williaster @alecklandgraf

Description

This adds filters to the ImageViewer component to adjust image brightness and contrast to complement the existing RotateControls and ZoomControls.

Motivation and Context

This will be used to further enhance image editing capabilities in the ImageViewer.

Testing

Local storybook

Screenshots

High brightness:

image

Low brightness:

image

Checklist

  • My code follows the style guide of this project.
  • I have updated or added documentation accordingly.
  • I have read the CONTRIBUTING document.

@airbnb-bot
Copy link
Collaborator

airbnb-bot commented Oct 9, 2020

Size Changes

Package Diff ESM Prev ESM CJS Prev CJS
core +0.6% 570.22 KB 566.86 KB 715.13 KB 709.87 KB

Compared to master. File sizes are unminified and ungzipped.

View raw build stats

Previous (master)

{
  "apollo": {
    "esm": 10832,
    "lib": 14147
  },
  "app-shell": {
    "esm": 12906,
    "lib": 19874
  },
  "composer": {
    "esm": 68247,
    "lib": 101805
  },
  "core": {
    "esm": 580467,
    "lib": 726903
  },
  "forms": {
    "esm": 37350,
    "lib": 49298
  },
  "icons": {
    "esm": 156355,
    "lib": 205626
  },
  "layouts": {
    "esm": 15298,
    "lib": 20770
  },
  "metrics": {
    "esm": 5467,
    "lib": 7729
  },
  "test-utils": {
    "esm": 4279,
    "lib": 5937
  }
}

Current

{
  "apollo": {
    "esm": 10832,
    "lib": 14147
  },
  "app-shell": {
    "esm": 12906,
    "lib": 19874
  },
  "composer": {
    "esm": 68247,
    "lib": 101805
  },
  "core": {
    "esm": 583901,
    "lib": 732297
  },
  "forms": {
    "esm": 37350,
    "lib": 49298
  },
  "icons": {
    "esm": 156355,
    "lib": 205626
  },
  "layouts": {
    "esm": 15298,
    "lib": 20770
  },
  "metrics": {
    "esm": 5467,
    "lib": 7729
  },
  "test-utils": {
    "esm": 4279,
    "lib": 5937
  }
}

@mheitman mheitman changed the title Add FilterControls component to ImageViewer feature(ImageViewer): Add FilterControls component to ImageViewer Oct 9, 2020
@mheitman
Copy link
Collaborator Author

@williaster @alecklandgraf PTAL 🙂

Copy link
Contributor

@williaster williaster left a comment

Choose a reason for hiding this comment

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

Looks great overall, had a couple minor comments on prop annotations

export type FilterControlsProps = {
/** The current brightness. 1 by default. */
brightness?: number;
/** Callback when brightness changes */
Copy link
Contributor

Choose a reason for hiding this comment

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

missing .

onBrightnessChange: (brightness: number) => void;
/** The current contrast. 1 by default. */
contrast?: number;
/** Callback when contrast changes */
Copy link
Contributor

Choose a reason for hiding this comment

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

missing .

onContrastChange: (contrast: number) => void;
/** Size of the icons. */
iconSize?: number | string;
/** Place dropdown menu above */
Copy link
Contributor

Choose a reason for hiding this comment

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

missing .

import useStyles, { StyleSheet } from '../../hooks/useStyles';

export type FilterControlsProps = {
/** The current brightness. 1 by default. */
Copy link
Contributor

Choose a reason for hiding this comment

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

might be worth mentioning what the valid range of brightness values is?

brightness?: number;
/** Callback when brightness changes */
onBrightnessChange: (brightness: number) => void;
/** The current contrast. 1 by default. */
Copy link
Contributor

Choose a reason for hiding this comment

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

might be worth mentioning what the valid range of contrast values is?

@mheitman
Copy link
Collaborator Author

@williaster updated the comments -- PTAL!

@williaster williaster merged commit 98f26c4 into master Nov 24, 2020
@williaster williaster deleted the mae_heitmann--add-contrast-picker branch November 24, 2020 01:23
@mheitman mheitman restored the mae_heitmann--add-contrast-picker branch November 25, 2020 23:21
@mheitman mheitman deleted the mae_heitmann--add-contrast-picker branch November 25, 2020 23:21
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.

3 participants