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: inject header right #1317

Open
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

ClaudeArs
Copy link

Summary of Changes

This PR makes all header buttons injectable

Related Issues

N/A

Pull Request Checklist

Tick all boxes below to demonstrate that you have completed the respective task. If the item does not apply to your this PR check it anyway to make it apparent that there's nothing to do.

  • All commits contain a DCO Signed-off-by line (we use the DCO GitHub app to enforce this);
  • Updated LICENSE-3RD-PARTY.md for any added dependencies or vendored components;
  • Updated documentation as needed for changed code and new or modified features;
  • Added sufficient tests so that overall code coverage is not reduced.

If you have any questions to any of the points above, just submit and ask! This checklist is here to help you, not to deter you from contributing!

Pro Tip 🤓

  • Read our contribution guide at least once; it will save you a few review cycles!
  • Your PR will likely not be reviewed until all the above boxes are checked and all automated tests have passed.

PR template adapted from the Python attrs project.

@bryce-mcmath bryce-mcmath changed the title Feat/inject header right feat: inject header right Nov 19, 2024
Copy link
Contributor

@bryce-mcmath bryce-mcmath left a comment

Choose a reason for hiding this comment

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

This is a better approach! My only two gripes are:

  1. These are actually screen options, not stack options. If we call them stack options then we will have two separate and very different defaultStackOptions variables in our codebase.
  2. You've moved some of the screen options to the defaultStackOptions but not all of them. It might be good to either keep them all in place, or move them all to the new defaultStackOptions. I would lean towards leaving them all in place.

Copy link

sonarcloud bot commented Nov 19, 2024

@MosCD3
Copy link
Contributor

MosCD3 commented Nov 19, 2024

Does have some overlap with #1321 could work out a complete solution
I am not in a favour of mixing any stackOptions with screen options

@bryce-mcmath
Copy link
Contributor

Ah right @ClaudeArs sorry I forgot a version of this already exists using dictionaries. See packages/legacy/core/App/navigators/defaultStackOptions.tsx and OnboardingStack. Might be good to have a chat with @MosCD3 as he was the one who implemented the first approach.

@ClaudeArs
Copy link
Author

Ok @MosCD3 , the goal of this PR is to give the possibility to inject a button in the right part of the header. According to you, what would be the best way to proceed?

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.

4 participants