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: Add sections to onChange props #260

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

nitaking
Copy link

@nitaking nitaking commented Nov 11, 2018

When I tried to create a menu component using Accordion, I attempted to implement Accordion in Accordion.

The use case is below.

  1. Perform specific processing at onChange (onPress)
    • At that time Accordion will not open
    • Which menu was pressed is determined by the index of onChange (index)
  2. When there is a nest Accordion
    • Open Accordion with onChange

In such a case, you can not distinguish nested menus with the onChange index alone.
So, we added sections to props on onChange.
This section represents sections in the same hierarchy as onChange.


I thought that this problem can be implemented if we use <Collapsible> without using <Accordion>. However, in that case I thought that it was redundant, implementing a feature like <Accordion> in <Collapsible>.

I feel that adding the above props is smartest, but what about it?

@nitaking
Copy link
Author

example source code.

const list = [
  {
    path: '',
    image: '',
    title: '',
    subMenuList: [
      {
        path: '',
        title: '',
        uri: '',
      },
      {
        path: '',
        title: '',
        uri: '',
      },
    ],
  },
];
...

export class SubMenuContents extends Component<Props> {
  state = {
    activeSections: [],
    activeSubSections: [],
  };

  onNestSection = (activeSections, sections) => {
    const activeMenu = sections[activeSections];
    goTo(activeMenu.path, { uri: activeMenu.uri });
  };

  setSections = (activeSections: $index[]) => {
    const { list } = this.props;

    this.setState({ activeSections });

    const pressMenu = activeSections.length ?
      list[activeSections] :
      list[this.state.activeSections];
    goTo(pressMenu.path, { uri: pressMenu.uri });
  };

  _render = (section) => {
    const { image, title } = section;
    return (
      <MenuItem title={title} image={image} />
    );
  };

  renderHeader = section => {
    return this._render(section);
  };

  renderContent = section => {
    if ('subMenuList' in section) {
      return (
        <Accordion
          sections={section.subMenuList}
          activeSections={this.state.activeSubSections}
          renderSectionTitle={() => <View />}
          renderHeader={this.renderHeader}
          renderContent={this.renderContent}
          onChange={this.onNestSection}
          touchableComponent={TouchableOpacity}
        />
      );
    }
    return <View />;
  };

  render() {
    const { list } = this.props;

    return (
      <Accordion
        sections={list}
        activeSections={this.state.activeSections}
        renderSectionTitle={() => <View />}
        renderHeader={this.renderHeader}
        renderContent={this.renderContent}
        onChange={this.setSections}
        touchableComponent={TouchableOpacity}
      />
    );
  }
}

@nitaking
Copy link
Author

@iRoachie how is it?

@iRoachie
Copy link
Collaborator

Correct me if I'm wrong. The sections param that you want to add is already passed in through props, why do you want to pass back the same sections prop in the onChange? Why not just use the variable that sets the sections prop directly?

@nitaking
Copy link
Author

@iRoachie
When nesting, I think that the same section is useless.

First we tried to use an already existing sections.
If you use it directly as a nested accordion, it will be in the following state.

const list = [
  { title: 'A' },
  {
    title: 'B',
    subList: [{ title: 'B-1' }, { title: 'B-2' } ],
  },
];

The result...

master branch:

displayTitle displayTitle
A
B A
B

The result I would like is...

this PR:

displayTitle displayTitle
A
B B-1
B-2

The reason is that I make the following reference.

master branch:

displayTitle index props.section displayTitle index props.section
A: list[0] 0 list
B: list[1] 1 list A: list[0] 0 list
B: list[1] 1 list

this PR:

displayTitle index props.section displayTitle index props.section
A: list[0] 0 list
B: list[1] 1 list B-1: list[1].subList[0] 0 subList
B-1: list[1].subList[1] 1 subList

@iRoachie
Copy link
Collaborator

Can you make a quick snack on http://snack.expo.io that i could test this with? I'll copy it locally and then use your branch/changes.

@nitaking
Copy link
Author

nitaking commented Feb 14, 2019

@iRoachie
Hi.
It is not snack, but I made a mock with codesandbox.
I could not use forked repo, but you touch the problematic phenomenon.
Please press title 5 under title 4.

https://codesandbox.io/s/qz0zrlkyrq

@nitaking
Copy link
Author

nitaking commented Feb 14, 2019

2019-02-15 02 18 20
I need sub list value.

@iRoachie
Copy link
Collaborator

After I apply your change, how would the example change to be able to get the information for the sublist?

@nitaking
Copy link
Author

@iRoachie
check this #260 (comment)

@nitaking
Copy link
Author

I think this is a change that should be merged soon.

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.

2 participants