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.

Sign up for GitHub

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

Jump to bottom

feat: support loading indicator for FAB #985

Merged
merged 5 commits into from
May 9, 2019
Merged

feat: support loading indicator for FAB #985

merged 5 commits into from
May 9, 2019

Conversation

sritharanpalani
Copy link
Contributor

Motivation

It is nice to have a loading indicator for a fab button

Test plan

1 - open example app in expo
2 -click on FAB example
3 -you can see the example of FAB with loading indicator as similar to a normal button
4 - try out ->

<FAB
    icon="cancel"
    label="Loading FAB"
    loading
    style={styles.fab}
    onPress={() => {}}
     visible={this.state.visible}
  />

@callstack-bot
Copy link

callstack-bot commented Apr 11, 2019 •
edited
Loading

Hey @sritharanpalani, thank you for your pull request 🤗. The documentation from this branch can be viewed here. Please remember to update Typescript types if you changed API.

ferrannp
ferrannp reviewed Apr 15, 2019
View reviewed changes
src/components/FAB/FAB.js Outdated
@@ -42,7 +42,11 @@ type Props = $RemoveChildren<typeof Surface> & {|
/**
* Whether `FAB` is currently visible.
*/
visible: boolean,
visible?: boolean,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Guessing this one does not need to be changed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

optional removed :)

@ferrannp
Copy link
Collaborator

@sritharanpalani can you add a screenshot or gif on how this looks?

Trancever
Trancever suggested changes Apr 30, 2019
View reviewed changes
src/components/FAB/FAB.js Outdated
@@ -256,6 +270,10 @@ const styles = StyleSheet.create({
disabled: {
elevation: 0,
},
icon: {
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove this, it's not needed

Copy link

Choose a reason for hiding this comment

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

Done.

src/components/FAB/FAB.js Outdated
{icon && loading !== true ? (
<CrossFadeIcon source={icon} size={24} color={foregroundColor} />
) : null}
{loading ? (
Copy link
Contributor

Choose a reason for hiding this comment

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

Only display it when label prop is specified (only for extended FAB)

Copy link

Choose a reason for hiding this comment

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

Done.

src/components/FAB/FAB.js Outdated
) : null}
{loading ? (
<ActivityIndicator
size="small"
Copy link
Contributor

Choose a reason for hiding this comment

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

Pass size={18}

Copy link

Choose a reason for hiding this comment

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

Done.

src/components/FAB/FAB.js Outdated
@@ -2,7 +2,7 @@

import color from 'color';
import * as React from 'react';
import { Animated, View, StyleSheet } from 'react-native';
import { Animated, View, StyleSheet, ActivityIndicator } from 'react-native';
Copy link
Contributor

Choose a reason for hiding this comment

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

Use ActivityIndicator from paper please.

Copy link

Choose a reason for hiding this comment

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

Done.

src/components/FAB/FAB.js Outdated
<ActivityIndicator
size="small"
color={foregroundColor}
style={styles.icon}
Copy link
Contributor

Choose a reason for hiding this comment

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

We don't need to pass any styles here

Copy link

Choose a reason for hiding this comment

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

Done.

@VivekNeel
refactor: address code review comments
a481b4e 
- refactor: remove icon from FAB.js
- fix: pass size 18 for ActivityIndicator in FAB.js
- fix: add FAB example for loading in FABExample.js
- fix: use ActivityIndicator from paper
- fix: show loadin only if label & loading is specified
@VivekNeel
Copy link

@ferrannp screenshot added, let me know if this is sufficient? And btw, apologize for addressing CR comments very late.

image

@Trancever Trancever merged commit 532e508 into callstack:master May 9, 2019
Trancever pushed a commit that referenced this pull request May 9, 2019
@sritharanpalani @Trancever
feat: support loading indicator for FAB (#985)
f6ac53f
iyadthayyil pushed a commit to iyadthayyil/react-native-paper that referenced this pull request Jun 6, 2019
@sritharanpalani @iyadthayyil
feat: support loading indicator for FAB (callstack#985)
a71664b
@fabian-astorga fabian-astorga mentioned this pull request Jul 3, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@VivekNeel VivekNeel VivekNeel left review comments

@ferrannp ferrannp ferrannp left review comments

@Trancever Trancever Trancever approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@sritharanpalani @callstack-bot @ferrannp @VivekNeel @Trancever