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

Account details update: made email & DoB mandatory and updated onboarding texts #5

Merged
merged 10 commits into from
Jan 24, 2024

Conversation

EvelynBunnyDev
Copy link
Contributor

@EvelynBunnyDev EvelynBunnyDev commented Jan 16, 2024

Name of the PR

♻️ Current situation & Problem

Modifying account detail configurations.
Responding to reviewer comment.

⚙️ Release Notes

Modified localizable strings (onboarding texts) and made the strings 'automatic'
Removed commented code that I'm not implementing.

📚 Documentation

In-line documentation provided.

✅ Testing

N/A. All elements should be testable

📝 Code of Conduct & Contributing Guidelines

By submitting creating this pull request, you agree to follow our Code of Conduct and Contributing Guidelines:

Copy link

codecov bot commented Jan 16, 2024

Codecov Report

Attention: 12 lines in your changes are missing coverage. Please review.

Comparison is base (063b54d) 41.07% compared to head (748f077) 40.90%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main       #5      +/-   ##
==========================================
- Coverage   41.07%   40.90%   -0.17%     
==========================================
  Files          30       30              
  Lines         957      966       +9     
==========================================
+ Hits          393      395       +2     
- Misses        564      571       +7     
Files Coverage Δ
Behavior/Account/AccountSheet.swift 0.00% <ø> (ø)
Behavior/BehaviorDelegate.swift 95.09% <100.00%> (+0.17%) ⬆️
Behavior/Account/AccountSetupHeader.swift 0.00% <0.00%> (ø)

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 063b54d...748f077. Read the comment docs.

1. Removed Vertical Whitespace before Closing Braces
2. Solved for Trailing Newline Violation
Copy link
Contributor

@dhruvna1k dhruvna1k left a comment

Choose a reason for hiding this comment

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

looks good

Copy link
Member

@Supereg Supereg left a comment

Choose a reason for hiding this comment

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

Hey @EvelynBunnyDev,

Thank you for the PR. It's a good first version that only needs some slight adjustments.

Your PR description does not follow the PR template that we introduced in the class, and that is provided by default: https://github.com/CS342/.github/blob/main/.github/pull_request_template.md.
Please follow the following template in your PR description and add a good description of your PR by editing the PR description on the PR page.
Great you already added a very descriptive title.

Please also read through the "Keeping your pull request in sync with the base branch" documentation to ensure that your branch is up to date with the main branch before you merge it into the main branch.

Please re-request a review once you have implemented all the requested changes.

Behavior/Account/AccountSetupHeader.swift Outdated Show resolved Hide resolved
Behavior/Account/AccountSetupHeader.swift Outdated Show resolved Hide resolved
Behavior/Account/AccountSheet.swift Show resolved Hide resolved
Behavior/Resources/Localizable.xcstrings Show resolved Hide resolved
Behavior/Resources/Localizable.xcstrings Outdated Show resolved Hide resolved
Behavior/BehaviorDelegate.swift Outdated Show resolved Hide resolved
Behavior/BehaviorDelegate.swift Outdated Show resolved Hide resolved
Copy link
Member

@Supereg Supereg left a comment

Choose a reason for hiding this comment

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

Just reviewed your PR again. It seems like you forgot to resolve some of the feedback I provided in my last review. Pointed them out below.

Behavior/Account/AccountSheet.swift Show resolved Hide resolved
Behavior/BehaviorDelegate.swift Outdated Show resolved Hide resolved
@EvelynBunnyDev EvelynBunnyDev self-assigned this Jan 22, 2024
Copy link
Member

@Supereg Supereg left a comment

Choose a reason for hiding this comment

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

Looks good 👍

@EvelynBunnyDev
Copy link
Contributor Author

Looks good 👍

Thank you, Andrea! Could you approve the PR?

Copy link
Member

@Supereg Supereg left a comment

Choose a reason for hiding this comment

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

Sorry, misclicked 🙈 there you go

@EvelynBunnyDev EvelynBunnyDev enabled auto-merge (squash) January 24, 2024 01:18
@EvelynBunnyDev EvelynBunnyDev merged commit a340ac1 into main Jan 24, 2024
7 checks passed
@EvelynBunnyDev EvelynBunnyDev deleted the acc-details branch January 24, 2024 01:47
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