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

feat: support max width parameter #141

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

Sineliad
Copy link

I tried in web version set a custom max width to the list, but I can't, so I add a new parameter to the component "maxWidth".

@naqvitalha
Copy link
Collaborator

Thanks for the pull request. If you can accept initial layout to the listview instead it would both achieve what you want here and help us skip one frame delay in load. In any case, this method will not work for canChangeSize={true}.

@mrbarletta
Copy link

Hi @naqvitalha - I am having a hard time understanding what it should be done to have this merged.

Do you mean adding instead an InitialLayout prop?

@naqvitalha
Copy link
Collaborator

@mrbarletta Yes exactly :)

@marnusw
Copy link
Contributor

marnusw commented Jun 4, 2018

@naqvitalha I could really use this functionality to implement a list within a Bootstrap grid container. It looks like there hasn't been any action here for a while. I'd be happy to pitch in, but I'm not clear on precisely what the initialLayout prop would do?

@mrbarletta are you still actively looking into this issue?

@mrbarletta
Copy link

@marnusw - if you know how to do it go ahead, I don't fully understand the whole library and don't want to make it slower adding frames.

we are using a patched version but would love to have this added, if you a better JS dev please give it a push

@marnusw
Copy link
Contributor

marnusw commented Jun 4, 2018

Thanks for the note @mrbarletta. We are cherry-picking your change into our own patch right now as well, but if I hear back from the maintainer and we can discuss the desired implementation I will give it a shot.

@naqvitalha
Copy link
Collaborator

@marnusw What exactly are you trying to achieve? This PR was about maxWidth but the given solution will actually not work in all cases. With the new Layout manager changes there might be a way to override widths already.

@marnusw
Copy link
Contributor

marnusw commented Jun 5, 2018

Our site uses the Bootstrap grid system, and so the width of our page content is less than the window width on tablets and desktop devices. The grid is still scrolled with the window scroll though.

We want to use RLV to create a long list of items within the grid. At the moment we only have two options:

  1. Use the container div for scroll, but then you get a scrollbar in the container independent of the window scroll which is not desired.
  2. Use window scroll, but then the list width is automatically set to the full window width and there is no way to override it.

So we are looking for the best of both worlds: Using window scroll, but also limiting the width of the list to that of the container.

@tafelito
Copy link
Contributor

tafelito commented Jun 6, 2018

Why the need of a maxWidth? Can't you wrap the rlv on a container with a max width instead? And then if you need it to be responsive (web) it could be achieved by using onLayout and changing the layoutProvider. Something like this

  const MAX_WIDTH = 736;

  handleLayout = ({ nativeEvent }) => {
    const { height: layoutHeight, width: layoutWidth } = nativeEvent.layout;
    const maxWidth = Math.min(layoutWidth, MAX_WIDTH);
    this.setState({
      height: layoutHeight,
      width: maxWidth,
      layoutProvider: LayoutUtil.getLayoutProvider(maxWidth),
    });
  };

  render() {
    const { width, height } = this.state;
    return (
      <View 
        style={{flex: 1, justifyContent: "center", alignItems: "center"}} 
        onLayout={this.handleLayout}
      >
        <View style={{ height,  width }} >
            {this.state.count > 0
              ? <RecyclerListView
                  style={{ flex: 1 }}
                  onEndReached={this.handleListEnd}
                  dataProvider={this.state.dataProvider}
                  layoutProvider={this.state.layoutProvider}
                  rowRenderer={this.rowRenderer}
                  renderFooter={this.renderFooter}
                  canChangeSize={true}
                />
              : null}
        </View>
      </View>
    );
  }
}

@mrbarletta
Copy link

@tafelito - its hardcoded to use window size.

 this.props.onSizeChanged({ height: window.innerHeight, width: window.innerWidth });

@tafelito
Copy link
Contributor

tafelito commented Jun 7, 2018

I'm sorry not sure if I'm following, but implementing what I posted before, you'll get something like this

Container View width - 736px
image

Window width - 1440px
image

The only problem I see is that onEndReached keeps calling, but that might be a different issue

@marnusw
Copy link
Contributor

marnusw commented Jun 7, 2018

@tafelito this issue is applicable in particular to usage on the web (i.e. not React-Native) when useWindowScroll is true. The line @mrbarletta referred to above is in the web ScrollView Component.

From what I've tried thus far there doesn't seem to be a way to fix the width inside a wrapper. Even when you do the layout is calculated on window width and thus extends outside the wrapper component. What the maxWidth property does in this PR is to override the width used for the for the layout when it is provided.

I just realised there is a bug in this implementation though: maxWidth should not always override window width, only when the maxWidth value is less than the window.innerWidth. Something similar to your example will fix it: const width = Math.min(window.innerWidth, maxWidth);

@tafelito
Copy link
Contributor

tafelito commented Jun 8, 2018

@marnusw the example I shown above is web using rnw

@naqvitalha
Copy link
Collaborator

Folks, you can override these behaviours with an externalScrollView even today. You won't need a fork. BaseScrollView interface is expected to call onSizeChanged which lets RLV know its bounds. For window scrolling you can passing an external scrollview like:

class ExternalScrollView extends React.Component {
  componentWillMount() {
    if (this.props.onSizeChanged) {
      this.props.onSizeChanged({ height: window.innerHeight, width: 400 });
    }
  }
  scrollTo(arg) {
    this.refs.refS.scrollTo(arg);
  }
  render() {
    return <ScrollViewer ref="refS" {...this.props} 
            onSizeChanged={()=>{ /* Do nothing */}} />;
  }
}

Overrides are everywhere in RLV. With initialLayout prop I wanted to simplify this solution and also save one frame skip to compute bounds.
For more details check this quick sandbox I built: https://codesandbox.io/s/l2lkm9vyx9

@marnusw
Copy link
Contributor

marnusw commented Jun 8, 2018

This is great @naqvitalha! Thank you very much for taking the time to explain this and create the example. And thank you for a great library in the first place.

We are going to try this approach which seems like it will solve this for us.

@tafelito
Copy link
Contributor

tafelito commented Jun 8, 2018

@naqvitalha that doesn't allow you to make it responsive though. I mean adjust the width based on layout changes

and why do you need to import ScrollViewer from "recyclerlistview/dist/reactnative/platform/web/scrollcomponent/ScrollViewer.js"?

@naqvitalha
Copy link
Collaborator

@tafelito I think you already got how to make it response. onLayout being missing on most browsers was the problem on web. RNW is implementing it with ResizeObserver so future looks promising.

Since you are using RNW you don't need ScrollViewer. You can even pass regular RN ScrollView.

@tafelito
Copy link
Contributor

tafelito commented Jun 9, 2018

thanks @naqvitalha indeed that worked using ScrollView! And yes, this is looking good! Now I need to find a way to hide the scrollbars, but I think that's not implemented on rnw on scrollview yet and not being able to use pseudo-classes makes it harder

@tafelito
Copy link
Contributor

tafelito commented Aug 11, 2018

@naqvitalha have you figured out a way to use WindowScrolling on RNW? I was able to put a maxWidth to the list using ExternalScrollView as you mentioned before but I couldn't find a way of putting a extra view right on the side of the list. Is there any workaround that you can think of? Other than using absolute positioning of course

@AE0011
Copy link
Contributor

AE0011 commented Feb 27, 2019

@tafelito did u find any way to make scrollbar absolute or hide?

@jason076
Copy link

What is the reason for the rlv to ignore it's parent elements width? In my opinion this is not very intuitive. I would expect the list to expand to it's parents width and not to the full window size.

To replace the whole ScrollViewer with an externalScrollViewer is not a very elegant solution to workaround this behavior, because the ScrollViewer has more functionality than just setting the width, which I have to re implement by my self.

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

Successfully merging this pull request may close these issues.

7 participants