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

SVA 427 Add log in/out buttons to settings #212

Merged
merged 11 commits into from
Jul 11, 2024

Conversation

mirkiy
Copy link

@mirkiy mirkiy commented Jun 6, 2024

Description

  • Please include a summary of the change and which issue is fixed.
  • List any dependencies that are required for this change.

Fixes # (issue number)

Type of change

Please delete options that are not relevant

  • [] Bug fix (non-breaking change which fixes an issue)
  • [] New Feature (non-breaking change which adds functionality)
  • [] Breaking change(fix or feature that would cause existing functionality to not work as expected)
  • [] This change requires a documentation update

Testing locally

  • Add here anything someone would need to do to test your code locally, or delete this section if nothing needed
  • E.g. run npm install in the app, refresh the database, look at a particular project that has a video, add a .env file value (NB do not share any sensitive details here as PRs are public)

Checklist

  • [] My code follows the style guidelines of this project
  • [] I have performed a self-review of my own code
  • [] I have commented my code, particularly in hard-to-understand areas
  • [] I have removed any unnecessary comments or console logging
  • [] I have made corresponding changes to the documentation (if required)
  • [] I have addressed accessibility, if needed
  • [] I have followed best practices, e.g. NativeBase approaches and theming
  • [] I have checked the app in dark mode, if making front-end design changes
  • [] My changes generate no new warnings
  • [] I have added tests that prove my fix is effective or that my feature works
  • [] New and existing unit tests pass locally with my changes
  • [] I have updated the version numbers in package.json files in the app and/or api directories as needed

Copy link
Collaborator

@PammyLo PammyLo left a comment

Choose a reason for hiding this comment

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

Great job @mirkiy! Looks and works fine. Just a few little observations.

Side note - there appear to be contradictory designs regarding the colour of the button when a user is logged in, the design for the screen shows the error.100 red as you've used here but the design for the STA button component which the ticket also links to shows the primary pink and a pressed state. Perhaps a wee liaise with design team over this?

<Text fontSize="xs">Version {version}</Text>
<PrivacyAndTermsLinks />
{featureFlags.login ? (
Copy link
Collaborator

@PammyLo PammyLo Jun 8, 2024

Choose a reason for hiding this comment

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

  • The button should align to the bottom regardless of the content, for an example have a look in ProjectFullDetails.tsx, where it is placed outside the ScrollView to achieve this.
  • when the login feature flag is off then it doesn't spread quite over the screen as it does currently and it will probably be released behind the flag initially.

Copy link
Author

Choose a reason for hiding this comment

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

This has been fixed now, thank you :)

Copy link
Collaborator

Choose a reason for hiding this comment

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

The .expo folder is created when an Expo project is started using "expo start" command. It should also automatically add it to .gitignore but think it is a bug in SDK49 that it hasn't done so.

Please delete his folder - you can safely delete it because it does not contain any information that is relevant for other developers working on the project, it is specific to your machine and it's regenerated by expo when you start your app again.

Copy link
Author

Choose a reason for hiding this comment

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

Thanks for this, fixed now too :)

SETTINGS
</Heading>

<View style={{ marginTop: 48 }}>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Style is not need here, just marginTop={'48px'} will suffice :)

Copy link
Author

Choose a reason for hiding this comment

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

Thank fixed now too :)

@PammyLo PammyLo merged commit 5a26a14 into develop Jul 11, 2024
1 check passed
@PammyLo PammyLo deleted the SVA-427-login-logout-buttons-settings branch July 11, 2024 18:20
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