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(rna): add DefaultContent and override styles #3009

Merged
merged 3 commits into from
Nov 18, 2022

Conversation

calebpollman
Copy link
Member

@calebpollman calebpollman commented Nov 17, 2022

Description of changes

Add DefaultContent component to align component structure and styling across RNA subcomponents, as well as lay out the groundwork for passing style on the subcomponents

Issue #, if available

NA

Description of how you validated changes

Manually tested, unit tests

Checklist

  • PR description included
  • yarn test passes
  • Tests are updated
  • No side effects or sideEffects field updated
  • Relevant documentation is changed or added (and PR referenced)

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@changeset-bot
Copy link

changeset-bot bot commented Nov 17, 2022

⚠️ No Changeset found

Latest commit: f5f1d84

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@calebpollman calebpollman added the run-tests Adding this label will trigger tests to run label Nov 17, 2022
@github-actions github-actions bot removed the run-tests Adding this label will trigger tests to run label Nov 17, 2022
@calebpollman calebpollman temporarily deployed to ci November 17, 2022 20:53 Inactive
@calebpollman calebpollman temporarily deployed to ci November 17, 2022 20:53 Inactive
@calebpollman calebpollman temporarily deployed to ci November 17, 2022 20:53 Inactive
@calebpollman calebpollman temporarily deployed to ci November 17, 2022 20:53 Inactive
dbanksdesign
dbanksdesign previously approved these changes Nov 18, 2022
@calebpollman calebpollman marked this pull request as ready for review November 18, 2022 00:32
@calebpollman calebpollman requested a review from a team as a code owner November 18, 2022 00:32
Comment on lines +3 to 5
// import Example from '../features/Authenticator/Demo/Example';
import Example from '../features/Authenticator/Styles/Example';
// import { Demo as InAppDemo } from '../features/InAppMessaging';
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moving forward we will use this structure for RN Example apps so we have a consistent import structure for using .env variables to dynamically load Example apps

return <Button onPress={signOut}>Sign Out</Button>;
}

function App() {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just noting this example is not complete and will be used for the style prop handling example

Comment on lines +57 to +63
<Button
onPress={resendCode}
variant="secondary"
style={styles.buttonSecondary}
>
{getResendCodeText()}
</Button>
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moved this outside the Footer to align it with the other subcomponents

<FormFields isPending={isPending} fields={fieldsWithHandlers} />
{error ? <ErrorMessage>{error}</ErrorMessage> : null}
<Button onPress={handleFormSubmit} style={styles.buttonPrimary}>
{isPending ? 'Verifying...' : getVerifyText()}
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed the Verifying... to align with other Authenticator implementations

@calebpollman calebpollman temporarily deployed to ci November 18, 2022 00:38 Inactive
@calebpollman calebpollman temporarily deployed to ci November 18, 2022 00:38 Inactive
@calebpollman calebpollman temporarily deployed to ci November 18, 2022 00:38 Inactive
@calebpollman calebpollman temporarily deployed to ci November 18, 2022 00:38 Inactive
Comment on lines +11 to +14
function useThemedStyles<Style>(getStyle: (theme: StrictTheme) => Style) {
const theme = useTheme();
return useMemo(() => getStyle(theme), [getStyle, theme]);
}
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will move this to this to live with theming in a follow up, needs to be able to handle additional params

buttonPrimary: {
marginVertical: space.medium,
},
buttonPrimaryLabel: {}, // themed value only
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These empty objects are temporary until we have added subcomponent theming support

'error' | 'Footer' | 'isPending'
> & {
buttons: DefaultButtons;
body?: React.ReactNode;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs to be retyped/refactored so it can handle style props for subcomponents that require unique content, for example SetupTOTP

return censoredVal;
};

const DefaultFormFields: DefaultRadioFormFieldsComponent = ({
Copy link
Member Author

@calebpollman calebpollman Nov 18, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should have been named DefaultRadioFormFields, will address in a follow up PR

level = 3,
...rest
}: DefaultHeaderProps): JSX.Element | null {
return children ? (
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only render the Heading when a child is provided to prevent whitespace from being rendered

@@ -19,6 +19,7 @@ export const defaultTexts = {
ENTER_USERNAME: 'Enter your username',
FAMILY_NAME: 'Family Name',
GIVEN_NAME: 'Given Name',
FORGOT_PASSWORD: 'Forgot Password?',
Copy link
Member Author

@calebpollman calebpollman Nov 18, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added a "short" version to improve the layout of the secondary buttons in the SignIn component:

@@ -16,7 +16,6 @@ export const getThemedStyles = (theme: StrictTheme): ErrorMessageStyles => {
flexDirection: 'row',
paddingHorizontal: space.xs,
paddingVertical: space.xl,
width: '100%',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice catch!

fieldErrorStyle?: FieldErrorsProps['style'];
fieldLabelStyle?: StyleProp<TextStyle>;
fieldStyle?: StyleProp<TextStyle>;
style?: StyleProp<ViewStyle>;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we piggyback a fieldRequiredStyle? :) I can add it as follow-up as well.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes! Good call (separate PR preferred)

@@ -9,7 +9,7 @@ import { name as appName } from './app.json';
import { setupStorybook } from './storybook';

//TODO: replace with env var
const initStorybook = false;
const initStorybook = true;
Copy link
Contributor

@ioanabrooks ioanabrooks Nov 18, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Assuming this should not go in

buttonPrimaryLabel: {}, // themed value only
buttonSecondary: {
marginVertical: space.xs,
minWidth: '50%',
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is to allow the secondary buttons to "stack" if scenarios where button labels are long-ish

@calebpollman calebpollman temporarily deployed to ci November 18, 2022 01:22 Inactive
@calebpollman calebpollman temporarily deployed to ci November 18, 2022 01:22 Inactive
@calebpollman calebpollman temporarily deployed to ci November 18, 2022 02:22 Inactive
@calebpollman calebpollman temporarily deployed to ci November 18, 2022 02:22 Inactive
@calebpollman calebpollman temporarily deployed to ci November 18, 2022 02:22 Inactive
Copy link
Contributor

@dbanksdesign dbanksdesign left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@calebpollman calebpollman temporarily deployed to ci November 18, 2022 03:09 Inactive
@calebpollman calebpollman temporarily deployed to ci November 18, 2022 03:09 Inactive
@calebpollman calebpollman temporarily deployed to ci November 18, 2022 03:09 Inactive
@calebpollman calebpollman temporarily deployed to ci November 18, 2022 03:09 Inactive
@calebpollman calebpollman merged commit b2a6e02 into rna/main Nov 18, 2022
@calebpollman calebpollman deleted the rna/add-rna-styles branch November 18, 2022 03:30
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.

3 participants