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

Using 'drawerScreenWidth' prop on landscape orientation #44

Open
Danite opened this issue May 10, 2021 · 6 comments
Open

Using 'drawerScreenWidth' prop on landscape orientation #44

Danite opened this issue May 10, 2021 · 6 comments

Comments

@Danite
Copy link

Danite commented May 10, 2021

I was wondering if there was a reason for setting the drawer width to a constant when the orientation is landscape and ignoring the drawerScreenWidth prop?

this.drawerWidth = this.isLandscape()
        ? MaxWidthOnLandscapeMode // <-- This constant
        : _resolveDrawerSize(props.drawerScreenWidth, this.screenWidth);
@lukebrandonfarrell
Copy link
Owner

Hmm, I'm not familiar with the reasoning behind this, maybe @rodriigovieira can answer that?

@definder
Copy link

Hello! I would like to add that drawerScreenHeight doesn't react on any value also.

@rodriigovieira
Copy link
Contributor

@Danite I'll investigate it next week, to see if there's something to be improved there.

Hello! I would like to add that drawerScreenHeight doesn't react on any value also.

Did that present any issues?

@rodriigovieira
Copy link
Contributor

@Danite I don't think the same width should be applied to the drawer when it is in both directions. For example, you might specify a width to use on landscape mode that's too big to be used on portrait mode, or a small width in portrait mode that would look weirdly small on landscape mode.

Ideally, I believe we should have two properties, one to define the width in landscape mode, and another to define it in portrait mode (this one we already have).

Sadly, I don't have time at this moment to work on this feature, but we're always welcome to PRs!

@lukebrandonfarrell
Copy link
Owner

@Danite if you make a PR for this, I'll be happy to review

@Danite
Copy link
Author

Danite commented Jun 1, 2021

@rodriigovieira That's a good point, I also think that it would be better to have 2 properties for this kind of configuration, do you think that it should be defined also for the height? Thanks for the answer!

@lukebrandonfarrell I'll take a look!

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

No branches or pull requests

4 participants