Skip to content

feat: Snackbar component#305

Merged
satya164 merged 1 commit into
masterfrom
feat/snackbar
May 1, 2018
Merged

feat: Snackbar component#305
satya164 merged 1 commit into
masterfrom
feat/snackbar

Conversation

@Trancever
Copy link
Copy Markdown
Contributor

Closes #16

@Trancever Trancever added the wip label Apr 9, 2018
@callstack-bot
Copy link
Copy Markdown

callstack-bot commented Apr 9, 2018

Hey @Trancever, thank you for your pull request 🤗. The documentation from this branch can be viewed here.

@Trancever Trancever requested review from ferrannp and satya164 April 9, 2018 19:09
@Trancever Trancever changed the title WIP: feat/SnackBar component feat/SnackBar component Apr 9, 2018
@Trancever Trancever removed the wip label Apr 9, 2018
@vladikoff
Copy link
Copy Markdown

I am excited for this 😄

@Trancever Trancever changed the title feat/SnackBar component feat: SnackBar component Apr 10, 2018
Comment thread src/styles/colors.js Outdated
export const grey600 = '#757575';
export const grey700 = '#616161';
export const grey800 = '#424242';
export const grey850 = '#323232';
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Since our colors file is just material design colors, maybe move this to the component? We do that for other colors.

Comment thread src/components/SnackBar.js Outdated
* });
* ```
*/
class SnackBar extends React.Component<Props, State> {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Let's name it Snackbar since it's a single word, like Toolbar

Comment thread src/components/SnackBar.js Outdated
/**
* Text that will be displayed inside SnackBar.
*/
content: string,
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Maybe use the children prop for this?

* - `onPress` - Callback that is called when action button is pressed, user needs to update the `visible` prop.
* - `color` - Color of the action button
*/
action?: {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Maybe we want to support multiple actions?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

MD docs say that only one action is allowed, so that's why i did it like that. Do we really want to support more than one?

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Yeah 0-1 actions is ok.

Comment thread src/components/SnackBar.js Outdated
action?: {
text: string,
onPress: () => mixed,
color?: string,
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Probably unnecessary since people can already override the accent color with the theme prop.

Comment thread src/components/SnackBar.js Outdated
duration: SNACKBAR_ANIMATION_DURATION,
useNativeDriver: true,
}),
]).start(this.props.onDismiss);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Since we're calling hide on componentDidUpdate, we shouldn't call this.props.onDismiss again.

Comment thread src/components/SnackBar.js Outdated
const { duration } = this.props;
if (duration !== 'indefinite') {
this.hideTimeout = setTimeout(
this.hide,
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Let's call this.props.onDismiss here instead of hide. componentDidUpdate will trigger the animation.

Comment thread src/components/SnackBar.js Outdated
};
}

hideTimeout: number;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Use _ prefix for private properties and methods.

Comment thread src/components/SnackBar.js Outdated
},
]}
>
<Animated.Text
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This shouldn't need Animated

<TouchableWithoutFeedback
onPress={() => {
this.hide();
action.onPress();
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Maybe call onPress first, then hide

Comment thread src/components/SnackBar.js Outdated
>
<View>
<Text style={{ color: action.color || colors.accent }}>
{action.text}
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This needs to be uppercase. You can use our own Button with compact (I think).

Copy link
Copy Markdown
Contributor Author

@Trancever Trancever Apr 16, 2018

Choose a reason for hiding this comment

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

I used our button and forgot to add toUpperCase when I changed it, will fix it. I decided not to use button from paper cause it has padding and margin by default and styling was not so clear and also it has ripple effect that we don't want in snackbar.

Comment thread src/components/SnackBar.js Outdated
const contentRightMargin = action ? 0 : 24;

return (
<Animated.View
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Can you wrap everything with ThemedPortal

Comment thread src/components/SnackBar.js Outdated
};

_hide = () => {
if (this._hideTimeout) {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Let's clear this on unmount too

@Trancever Trancever added the wip label Apr 17, 2018
Comment thread src/components/Snackbar.js Outdated
const contentRightMargin = action ? 0 : 24;

return (
<ThemedPortal theme={theme}>
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

You don't need to pass theme here

Comment thread example/src/ExampleList.js Outdated
import RadioButtonGroupExample from './RadioButtonGroupExample';
import RippleExample from './RippleExample';
import SearchBarExample from './SearchBarExample';
import SnackBarExample from './SnackBarExample';
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Let's rename this too :)

Comment thread example/src/SnackBarExample.js Outdated
};

class SnackBarExample extends React.Component<Props, State> {
static title = 'Snack bar';
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

nit: Snackbar

Comment thread example/src/SnackBarExample.js Outdated
}}
duration={Snackbar.DURATION_INDEFINITE}
>
<Text>Put your message here</Text>
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Maybe don't need the wrapper <Text>

@@ -0,0 +1,292 @@
/* @flow */
import * as React from 'react';
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

nit: space before

Comment thread src/components/Snackbar.js Outdated
/**
* Text that will be displayed inside SnackBar.
*/
children: string | React.Node,
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

React.Node should already include string

Comment thread src/components/Snackbar.js Outdated
* Object that determines button text and callback for button press. It should contains following properties:
* - `text` - Content of the action button
* - `onPress` - Callback that is called when action button is pressed, user needs to update the `visible` prop.
* - `color` - Color of the action button
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Unused :)

Comment thread src/components/Snackbar.js Outdated
*/
duration?: number,
/**
* Callback called when Snackbar is dismissed, user needs to update the `visible` prop.
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Callback called when Snackbar is dismissed. The `visible` prop needs to be updated when this is called.

@Trancever Trancever removed the wip label Apr 20, 2018
@Trancever Trancever changed the title feat: SnackBar component feat: Snackbar component Apr 20, 2018
@vladikoff
Copy link
Copy Markdown

I'm trying to use this but getting this:

I'm not using any theme stuff, that might be the cause, investigating....

@vladikoff
Copy link
Copy Markdown

My render fn:

  render() {
    
    return (
      <View style={{ flex: 1 }}>
        { this.renderList() }

        <FAB
          small
          color={COLOR_WHITE}
          style={styles.fab}
          icon="add"
          onPress={() => this.new()}
        />

        <Snackbar
          visible={this.state.visible}
          onDismiss={() => this.setState({ visible: false })}
          action={{
            text: 'Undo',
            onPress: () => {
              this.setState({ visible: false });
            },
          }}
          duration={Snackbar.DURATION_INDEFINITE}
        >
          Put your message here
        </Snackbar>
      </View>
    );
  }

@satya164
Copy link
Copy Markdown
Member

@vladikoff did you follow the getting started guide? you need a <Provider /> at root of your app.

@vladikoff
Copy link
Copy Markdown

did you follow the getting started guide? you need a at root of your app.

Yeah I have the rest of paper working. Just trying to add a snackbar from this Pull Request

  render () {
    return (
      <Provider store={store}>
        <App/>
      </Provider>
    )
  }

Removing <ThemedPortal> from Snackbar.js helps remove the red crash but the Snackbar is not visible at the moment.

@vladikoff
Copy link
Copy Markdown

@satya164 oh I have import { Provider } from 'react-redux'; not Provider as PaperProvider should I use the PaperProvider?

@vladikoff
Copy link
Copy Markdown

Nvm, let me revisit https://callstack.github.io/react-native-paper/getting-started.html again , sorry

@vladikoff
Copy link
Copy Markdown

Cool it's working here! I just need to move my fab up now :)

However, note that if you set the initial state to:

  constructor(props) {
    super(props);
    this.props = props;
    this.state = {
      visible: true // <---------- TRUE here
    }

then it will not show the Snackbar when the view renders for the first time, not sure if bug or something with my setup. Thanks again!

@Trancever
Copy link
Copy Markdown
Contributor Author

@vladikoff Cool that it's working! We are planning to add support for automatically moving FAB whenever Snackbar shows so stay tuned 😄

@vladikoff
Copy link
Copy Markdown

Sorry for a stupid question but what controls the stacking order (like css z-index) for this?

For example if I have a drawer, how do I make sure the Snackbar doesn't overlay it?

Example:

@satya164
Copy link
Copy Markdown
Member

what controls the stacking order (like css z-index) for this

It's controlled by the Provider component. If you place the drawer inside Provider, the drawer will be below the Snack and vice-versa.

@vladikoff
Copy link
Copy Markdown

It's controlled by the Provider component. If you place the drawer inside Provider, the drawer will be below the Snack and vice-versa.

Ah right, that helps! I tried doing the animation of the FAB and the Snackbar together but it is very choppy. Probably because it is not using the Animated.parallel call

@Trancever
Copy link
Copy Markdown
Contributor Author

@vladikoff There is a description in a Portal issue #43 which says how those components (Snackbar, FAB, etc.) should behave together and how we would like to implement it. You can check it out.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants