Skip to content

Code cleanup #33

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

Merged
merged 13 commits into from
Apr 11, 2023
Merged

Conversation

Thodor12
Copy link
Contributor

@Thodor12 Thodor12 commented Apr 6, 2023

Revamped the entire documentation to better reflect all the properties, aswell as describe newly added properties.
Code changes to remove unnecessary properties, merged certain properties into objects, used proper typings where possible.

Property renames:

  • checkboxColor -> checkboxProps.checkboxColor
  • checkboxUncheckedColor -> checkboxProps.checkboxUncheckedColor
  • checkboxLabelStyle -> checkboxProps.checkboxLabelStyle
  • checkboxLabelStyle -> checkboxProps.checkboxLabelStyle
  • searchPlaceholder -> searchText

Property type changes:

  • searchStyle changed from custom object to ViewStyle
  • dialogStyle changed from custom object to ViewStyle
  • theme changed from custom object to ThemeProp (no effective change)

Other changes:

  • textInputMode now defaults to "flat" to follow react-native-paper defaults
  • "Select All" changed to "Select all"
  • Error text now utilizes the MUI error theme instead of falling back to "red", unless overridden
  • Theme now utilizes react native paper useInternalTheme to automatically grab the theme from the provider, giving the option to override the style on a per-component basis
  • Styles have all been extracted to non-inline styles, except for the ones which rely on variables

No functionality should work differently after this PR, short of property changes, so it will be a breaking change.
Users may see differences in styling in some places (for example the defaulting to "flat", better respecting MUI theme, etc)

resolves #31

@Thodor12
Copy link
Contributor Author

Thodor12 commented Apr 6, 2023

First finish #32 so changes can be merged into here

@srivastavaanurag79
Copy link
Owner

Please add the issue #31 feature needed i.e. the left icon prop in the text input. And thank you for your work.

@Thodor12
Copy link
Contributor Author

Thodor12 commented Apr 7, 2023

@srivastavaanurag79 added that option too, could you look at #32 so I can merge that back into here?

@Thodor12
Copy link
Contributor Author

Thodor12 commented Apr 7, 2023

Dialog label changes have been ported into here instead:

Currently it's not possible to separately style the close and done buttons.
In my application I'm consistently holding to a "error" style for cancel buttons and a "primary" style for completion/save buttons.
This is not doable if there's only 1 property available for setting the style for both buttons.

Suggested changes:

  • Introduce separate properties for the done and close buttons
  • Rename matching text properties to refer to "dialog" instead of "modal", to match other properties
  • Thanks to prettier some formatting related things have been changed (blame prettier)

@Thodor12 Thodor12 marked this pull request as ready for review April 7, 2023 20:20
@srivastavaanurag79
Copy link
Owner

Error: Unable to resolve module react-native-paper/lib/typescript/src/core/theming

@srivastavaanurag79
Copy link
Owner

did you remove the icon and everything?

@srivastavaanurag79
Copy link
Owner

srivastavaanurag79 commented Apr 8, 2023

can you please attach some screenshots of what it looks like now?

@srivastavaanurag79
Copy link
Owner

srivastavaanurag79 commented Apr 8, 2023

why have you wrapped it again inside a provider? people who would add react-native-paper will add their main app inside provider.

@Thodor12
Copy link
Contributor Author

Thodor12 commented Apr 8, 2023

Error: Unable to resolve module react-native-paper/lib/typescript/src/core/theming

I pulled the repo and installed react-native-paper like normal, it could be there's a package update which prevents you from building it locally.

did you remove the icon and everything?

Some properties have been moved to textInputProps, checkboxProps and searchbarProps as they don't warrant having to make a first class additional property. No original functionality has been removed.

can you please attach some screenshots of what it looks like now?

Will do a bit later, is there a way I can actually run the app locally. Is it possible to run the example using local version of the code?

why have you wrapped it again inside a provider? people who would add react-native-paper will add their main app inside provider.

There's 2 different providers, PaperProvider and ThemeProvider. The first one is from react-native-paper themselves to supply the theme prop aswell as do other things underwater. ThemeProvider is the actual supply mechanism for the theme to MUI components. You can see that behaviour here: https://github.com/callstack/react-native-paper/blob/main/src/core/Provider.tsx and https://github.com/callstack/react-native-paper/blob/main/src/core/theming.tsx#L31

By doing it this way you don't have to manually pass theme={theme} to every single component as the react context will simply utilize the closest ThemeProvider.

@srivastavaanurag79
Copy link
Owner

srivastavaanurag79 commented Apr 8, 2023

You changed a lot of things, and it isn't working in my system. It keeps throwing the error I sent you react-native-paper v5.5.1 is installed for me previously I was using v4.9.2

@Thodor12
Copy link
Contributor Author

Thodor12 commented Apr 8, 2023

You changed a lot of things, and it isn't working in my system. It keeps throwing the error I sent you react-native-paper v5.5.1 is installed for me previously I was using v4.9.2

My version is 5.5.1 too, could you maybe clean your node_modules? The import is correct, also:

Will do a bit later, is there a way I can actually run the app locally. Is it possible to run the example using local version of the code?

@srivastavaanurag79
Copy link
Owner

You changed a lot of things, and it isn't working in my system. It keeps throwing the error I sent you react-native-paper v5.5.1 is installed for me previously I was using v4.9.2

My version is 5.5.1 too, could you maybe clean your node_modules? The import is correct, also:

Will do a bit later, is there a way I can actually run the app locally. Is it possible to run the example using local version of the code?

I am running the local version of the example and the error still persists. Done clean install still same error:

Error: Unable to resolve module react-native-paper/lib/typescript/src/core/theming

ThemeProvider can be imported directly from react-native-paper but the second import requires this path and its throwing error

@Thodor12
Copy link
Contributor Author

Thodor12 commented Apr 9, 2023

@srivastavaanurag79 I changed the imports and swapped useInternalTheme to useTheme. I cannot get the example to work on my local machine (it won't import the local changes to the package) so I am not able to actually check how it all works now

@srivastavaanurag79 srivastavaanurag79 merged commit c13c1fb into srivastavaanurag79:master Apr 11, 2023
@Thodor12 Thodor12 deleted the feature/cleanup branch April 11, 2023 15:39
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.

Support for left icon
2 participants