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

fix(ripple): focus style alive when disabled #943

Merged
merged 4 commits into from Jun 21, 2019
Merged

fix(ripple): focus style alive when disabled #943

merged 4 commits into from Jun 21, 2019

Conversation

ghost
Copy link

@ghost ghost commented Jun 20, 2019

fixes #916

@poying
I wrote it pretty much like your code.
But I'm not call handleBlur and I made disabledRipplePostProcessor.
Because there are some possibility that adds other code inhandleBlur.
If you have any other opinion, tell me about that!

@moog16
I added adapter to a member variable.
Existing member variables do not have an adapter, is there any special reason??

If both of you are okay, I'll write unit test code :)

@codecov-io
Copy link

codecov-io commented Jun 20, 2019

Codecov Report

Merging #943 into rc0.14.0 will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@             Coverage Diff              @@
##           rc0.14.0     #943      +/-   ##
============================================
+ Coverage     94.08%   94.08%   +<.01%     
============================================
  Files            86       86              
  Lines          3634     3637       +3     
  Branches        572      573       +1     
============================================
+ Hits           3419     3422       +3     
  Misses           90       90              
  Partials        125      125
Impacted Files Coverage Δ
packages/ripple/index.tsx 86.29% <100%> (+0.33%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f829e12...7d6468e. Read the comment docs.

packages/ripple/index.tsx Outdated Show resolved Hide resolved
import {withRipple, InjectedProps} from '../../../packages/ripple/index';

import Button from '../../../packages/button';
import {withRipple, InjectedProps} from '../../../packages/ripple';
Copy link

Choose a reason for hiding this comment

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

do you need this import here? It looks unused.

Copy link
Author

Choose a reason for hiding this comment

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

Yeah this is just my habit.
I always separate node_module and local_module imports!
Does it look better to put it back??

Copy link
Author

Choose a reason for hiding this comment

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

Maybe I misunderstood your comment.
This code already existed before.
import {withRipple, InjectedProps} from '../../../packages/ripple';
I'm just moved it down.

Copy link

Choose a reason for hiding this comment

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

ah! Sorry I didn't notice that. Yes that was there before. Looks good.

@moog16
Copy link

moog16 commented Jun 20, 2019

Tested this by using part of the stackblitz in the original issue:


class ButtonScreenshotTest extends React.Component<ButtState, {}> {

  state = {format: 'map'};
  render() {
    const {format} = this.state;

    return (
      <div>
    <div className="search-tools__format">
      <Button unelevated={format === "map"}
        icon={<MaterialIcon icon="map" />}
        disabled={format === "map"}
        onClick={() => {this.setState({ format: "map"})}} />
      <Button unelevated={format === "list"}
        icon={<MaterialIcon icon="view_list" />}
        disabled={format === "list"}
        onClick={() => {this.setState({ format: "list"})}} />
      <Button unelevated={format === "grid"}
        icon={<MaterialIcon icon="view_module" />}
        disabled={format === "grid"}
        onClick={() => {this.setState({ format: "grid"})}} />
    </div>
)
}
}

Looks like its working!

Copy link

@moog16 moog16 left a comment

Choose a reason for hiding this comment

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

one small thing

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants