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

[DFC-661]: Remove the Beta Banner from Strategic App view #2117

Merged

Conversation

SophiaDWokoma
Copy link
Contributor

@SophiaDWokoma SophiaDWokoma commented Sep 30, 2024

What

Implemented conditional rendering of the beta banner in the base.njk file to ensure it is only visible in web view. Also, added integration tests to verify the functionality.

How to review

  1. Code Review
  2. Set DEFAULT_CHANNEL in a local frontend .env file to either 'web' or 'strategic_app' and start the application.
  3. Verify if the beta banner correctly shows or hides based on the selected view.

Checklist

  • Performance analyst has been notified of the change.
  • A UCD review has been performed.

Related PRs

DFC-661

@SophiaDWokoma SophiaDWokoma requested review from a team as code owners September 30, 2024 15:37
@di-aholme
Copy link
Contributor

Internal sign off done - works locally as expected. Ready for handover to auth team

@SophiaDWokoma SophiaDWokoma changed the title [DFC-661]: Run precommit [DFC-661]: Remove the Beta Banner from Strategic App view Oct 3, 2024
@Joanneyoung01
Copy link
Contributor

Tested and verified locally - LGTM

Joanneyoung01
Joanneyoung01 previously approved these changes Oct 3, 2024
Copy link
Collaborator

@gtvj gtvj left a comment

Choose a reason for hiding this comment

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

Hi @SophiaDWokoma 👋🏽

Thank you for your work on this. I haven't yet reviewed the code or PR, but it seems but it seems some of the comments I've provided on #2114 (review) might be relevant here too. Would you be OK to look into these?

@gtvj
Copy link
Collaborator

gtvj commented Oct 7, 2024

Hi @SophiaDWokoma - thanks for your recent comment (and apologies that I'd started the thread in the wrong PR, I'd been doing a lot of context switching). I have moved the comment to the right place now and provided a bit more context.

We tend to avoid using "squash and merge" because it loses information that might be useful to future developers.

Copy link
Collaborator

@gtvj gtvj left a comment

Choose a reason for hiding this comment

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

Hi @SophiaDWokoma - I've made just a couple of suggestions on this PR relating to the way tests are described.

Like #2114, my sense is that the commits for this PR would benefit from being brought in line with The GDS Way - Working with Git guidance that I shared previously. There are four commits currently and, given this is a single step (i.e. hiding the phase banner), I'd have thought this would would probably be appropriate to include in a single commit.

@SophiaDWokoma
Copy link
Contributor Author

SophiaDWokoma commented Oct 15, 2024

Hi @SophiaDWokoma - I've made just a couple of suggestions on this PR relating to the way tests are described.

Like #2114, my sense is that the commits for this PR would benefit from being brought in line with The GDS Way - Working with Git guidance that I shared previously. There are four commits currently and, given this is a single step (i.e. hiding the phase banner), I'd have thought this would would probably be appropriate to include in a single commit.

Ok I have used fixup to clean up the commit history and combine the commits.

@SophiaDWokoma SophiaDWokoma force-pushed the DFC-661-Remove-Beta-Banner-from-Strategic-App-View branch from c415dd9 to 6c06f90 Compare October 16, 2024 15:01
@SophiaDWokoma SophiaDWokoma force-pushed the DFC-661-Remove-Beta-Banner-from-Strategic-App-View branch from 6c06f90 to c663cd5 Compare October 17, 2024 15:25
Copy link

sonarcloud bot commented Oct 17, 2024

@SophiaDWokoma SophiaDWokoma merged commit e5bd7f2 into main Oct 18, 2024
8 checks passed
@SophiaDWokoma SophiaDWokoma deleted the DFC-661-Remove-Beta-Banner-from-Strategic-App-View branch October 18, 2024 12:08
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