[Implement] : FAB

[Juyeong-Byeon]

Description

[implement] : FAB

1. Implemented the basic functions of the FAB
   * Can use IconButton that is a default button component of FAB by passing the icon types and callbacks.
   * Can replace the default button component of FAB by passing renderButton function
   * Can set absolute position of FAB
   * Can replace buttons by active Props

Rendering conditions:
if(active===false)render DefaultFAB
if(active===true)render ActiveFAB and ActionButtons

please comment about the structure of Component.

**Animation is not implemented**
**Need more implementation**

Related Issues

[FloatingActionButton] implementing floatingActionButton component

Tests

I did not add the test yet.
This PR needs more implementation.

Checklist

• I read the Contributor Guide and followed the process outlined there for submitting PRs.
• Run yarn test or yarn test -u if you need to update snapshot.
• Run yarn lint
• I am willing to follow-up on review comments in a timely manner.

[codecov]

Codecov Report

Merging #67 (afd59c4) into master (0d8318d) will decrease coverage by 1.44%.
The diff coverage is 96.29%.

@@            Coverage Diff             @@
##           master      #67      +/-   ##
==========================================
- Coverage   95.53%   94.08%   -1.45%     
==========================================
  Files          23       23              
  Lines         582      609      +27     
  Branches      267      275       +8     
==========================================
+ Hits          556      573      +17     
- Misses         26       36      +10     

[yujonglee]

Nice Work!

I slightly look into it. I think maybe it will be easier for me to understand if you provide some example code and output.
But It was totally fine.

Ok. As I understand, FABList is rendered only when Active. And DefaultFAB/ActiveFAB is chosen by Active state.
Also, ActiveFAB, DefaultFAB, and FABItem can have render function or it is rendered as default form.

I think overall structure is fine, but naming can be better.

1. I think DefaultFAB is the most confusing part. Default <--> Active should be replaced with Inactive <--> Active or Expanded <--> Collapsed. And the word Default should be used for without render function situation.
2. ActiveFAB and FABItem has Parent-Children relation, but it is not revealed by variable name.

---

BTW, this might be silly mistake of mine, but I'm seeing icon never changes.
I'm always seeing same icon whenever I change iconName string.

I'm using like this.

<FAB
  active={true}
  ActiveFAB={{icon: 'cross-light', onPress: () => null}}
  DefaultFAB={{icon: 'cross-light', onPress: () => null}}
  FABList={[
    {icon: 'chevron-down-light', onPress: () => null},
    {icon: 'chevron-down-light', onPress: () => null},
    {icon: 'chevron-down-light', onPress: () => null},
  ]}
/>

[yujonglee]

It's hard to communicate many changes at once with review(and codecov warning is annoying 😄 ), so I pasted code below.
This is just a suggestion. Feel free to point out anything.

1. My main goal was make code self-explainary by removing conditionals.
2. I can't be sure this refactor guarantee same functionality as before because there's no test code. But I think It is sufficient for showing my intention.
3. theme is not provided to defaultSubRenderer in code below. It should be changed. It's just a demo :)

// Above code untouched
const defaultRootRenderer = ({icon, onPress}: FABItem): React.ReactElement => (
  <IconButton icon={<StyledIcon size={24} name={icon} />} onPress={onPress} />
);

const defaultSubRenderer = (
  {icon, onPress}: FABItem,
  _index: number,
): React.ReactElement => (
  <IconButton
    icon={<StyledIcon theme={theme} size={24} name={icon} />}
    onPress={onPress}
  />
);

const FloatingActionButtons: FC<
  FloatingActionButtonsWrapperProps & {theme: DoobooTheme}
> = ({
  theme,
  active,
  inactiveRootItem,
  activeRootItem,
  subItems,
  renderInactiveRootItem = defaultRootRenderer,
  renderActiveRootItem = defaultRootRenderer,
  renderSubItem = defaultSubRenderer,
  zIndex = 0,
  top = 'auto',
  bottom = 'auto',
  left = 'auto',
  right = 'auto',
}) => {
  return (
    <FABContainer
      style={{
        top,
        bottom,
        left,
        right,
        zIndex,
      }}>
      <Animated.View>
        {active &&
          subItems.map((item, idx) => (
            <ItemContainer key={item.icon + idx}>
              {renderSubItem(item, idx)}
            </ItemContainer>
          ))}
      </Animated.View>
      <ItemContainer>
        {active
          ? renderInactiveRootItem(inactiveRootItem)
          : renderActiveRootItem(activeRootItem)}
      </ItemContainer>
    </FABContainer>
  );
};

export const FAB = withTheme(FloatingActionButtons);

[hyochan]

Looks like @yujong-lee covered most of my review already which I've communicated earlier. Thanks a lot! You are such a quick learner 💯

[hyochan]

I think the spec would be simplified to below. How do you think 🤔 ?

  <FAB
    style={{ background: 'red' }}
    data={[ 'main', 'settings', 'addItem' ]}
    icon="plus"
    isActive={isActive}
    onPress={() => setIsActive(!isActive)}
    onPressItem={(data) => console.log('pressed', data)}
    renderFAB={() => <View/>}
    renderFABItem={() => <View/>}
  />

[hyochan]

Also the generic can be extended like below

interface FABItemProp {
  icon: string;
  name: string;
}

type Props<T extends FABItemProp> = {
  data: T[];

[Juyeong-Byeon]

@hyochan thanks
It is very good idea to use generic extended the FABItem.
It will give more flexibility to FAB component

[yujonglee]

but I think we should support default components such as with conditionals.
Because it makes it much easier to use components. It will improve developers' experience in use.

@wndudqus For sure. My code above supports default component, but I remove conditionals using default value feature which language provides.

// code from props destructing above
renderInactiveRootItem = defaultRootRenderer,
renderActiveRootItem = defaultRootRenderer,
renderSubItem = defaultSubRenderer,

Note that I set default render function that I defined above. User can provide render function, or omit it.

If there's anything I misunderstood, please let me know.

[Juyeong-Byeon]

@yujong-lee
Oh, I see... I think this approach seems to simplify the code.
How do you think? @hyochan

[hyochan]

@yujong-lee
Oh, I see... I think this approach seems to simplify the code.
How do you think? @hyochan

Yes for the default component~ You can try @yujong-lee 's proposal 👍

[hyochan]

@wndudqus Please rerequest the review when you think you are ready!

[yujonglee]

It's good to see you working on it continuously. 👍

I left some comments. Feel free to ask, or point it out if I'm wrong.

[yujonglee]

This is pattern from enzyme, which is not good practice for testing library.

I recommend writing like this.

const {getByTestId} = render(...);

fireEvent.press(getByTestId('item1'));

or

render(...);

fireEvent.press(screen.getByTestId('item1'));

Although later one is new feature and recommended by author, I rarely use it. We can talk about it if you want.

[yujonglee]

See this for detailed guide for using testing-library.

[yujonglee]

We should avoid using let if possible.
If you use jest.fn(), they are not needed.

See how I did it here.

You can use toBeCalled for this case.

[yujonglee]

Maybe async is used due to animation duration?

For that, I think jest.useFakeTimers() is available.

See this for detail.

[hyochan]

Great work~! I've just pointed small fixes on naming.

[hyochan]

When FAB first renders, it doesn't hide the children. It should not run the animation when it shouldn't and also should follow isActive prop.

When looking in your code I see that you are showing children with opacity animation.

I have some issues with the above design.

1. The FABItem should appear from the main FAB button but they are just appearing from their target positions. I think we need to change that.
2. As told previously, it should behave with actual prop value.

[hyochan]

Additionally, the custom render function would not want to receive the callback for the press event. It'd only want to provide a different UI element.

Below is just an example.

{FABList.map((item, idx) => {
  return (
    <TouchableOpacity onPress={onPressFABItem?.(item)}>
      {renderFABListItem ? (
        renderFABListItem(item, idx)
      ) : (
        <StyledIcon theme={theme} size={24} name={item.icon} />
      )}
    </TouchableOpacity>
  );
})}

[Juyeong-Byeon]

@hyochan @yujong-lee

I think I covered most of your reviews

1. change Props naming
2. changed behavior
   Previously, only the transparency of the button was adjusted depending on the state but now the actual position is adjusted.
   3. Change state management.
   Previously, the active was managed as a "state", modified to manage it as a "props".
   4. path name changed
   FloatingActionButtonWrapper> FAB
3. Bug that state was not synced with UI has fixed

Simulator.Screen.Recording.-.iPhone.12.Pro.Max.-.2021-09-16.at.22.38.29.mp4

[yujonglee]

I was impressed by your clean code. 👍

I just left few more suggestions you can try!

[yujonglee]

renderProps is not for providing ReactElement. It is for reusing render logic.

Like you did, Items - renderItem is proper use case here. Because it is providing render logic for each item. But If you are just providing ReactElement, children or slot pattern is for that use.

Additionally, it is better for performance too. Because ReactElement is always newly created for each render if you call () => ReactElement function. (Although It has almost no impact here)

[Juyeong-Byeon]

I think it is better to provide consistency on the methods on providing custom component.
Now in this component renderFABItem should be provided as renderProps so.. I think it is better to provide renderFAB as renderProps.

[yujonglee]

Maybe we can have additional styles prop?
One for Animated.View for main icon and one for items.

[yujonglee]

Although they might have same size in general, I think FABItem is responsible for setting size, not FloatingActionButtons component.

[yujonglee]

@wndudqus

There's still some parts that is not reflected in code review. For example, renderProp, jest.fn or styles.(buttonWrapperStyle is better replaced with styles.)

If some reviews are not acceptable, feel free to tell me and have a discussion. It should not be one-sided.

But since this pull request is old and big, let's take care of this in later PR.
Thank you for great work on this!

[Juyeong-Byeon]

@yujong-lee oh I left some comments before request a review, but I forgot to submit the review so.. my mistake sorry.
And thank you for reviewing my code!