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

Updated header (Nav Bar) for hiding reports page #2238

Merged
merged 11 commits into from
Sep 20, 2023

Conversation

courtneyc1
Copy link
Contributor

Hiding Reports page
due to no current data being presented needed to hide this page until it gets updated.
frontend/src/components/Header.tsx
commented out line 250-255 to hide page.

🗣 Description

frontend/src/components/Header.tsx
comment out line 250-255 to hide the page.

Will need to uncomment when reports page is finished.

💭 Motivation and context

Clients get confused on reports page when nothing is presented currently.

gives dev team a chance to update the reports page

🧪 Testing

used local crossed to verify changes

Screenshot 2023-09-13 at 9 03 35 AM

before when reports page was present to

Screenshot 2023-09-13 at 9 35 24 AM

reports page hide on the nav bar.

commented out line 250-255 to hide page.
@courtneyc1 courtneyc1 linked an issue Sep 13, 2023 that may be closed by this pull request
2 tasks
@dmfezzareed dmfezzareed added the Frontend ASM-VDB User Interface label Sep 13, 2023
@Matthew-Grayson
Copy link
Contributor

@epicfaace Rachel asked Courtney to hide this page until it is functional. I'd like your input as to whether we should comment it out or remove it (based on how we typically handle unused code).

Copy link
Contributor

@epicfaace epicfaace left a comment

Choose a reason for hiding this comment

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

I would check with P&E team / Andy / Lamar before removing it. I know the page does include reports for federal agencies enrolled in P&E. If we need to use it for P&E, I’d suggest showing it only for govt agencies organizations and hiding it elsewhere.

@aloftus23 @stewartl97

@epicfaace
Copy link
Contributor

I would check with P&E team / Andy / Lamar before removing it. I know the page does include reports for federal agencies enrolled in P&E. If we need to use it for P&E, I’d suggest showing it only for govt agencies organizations and hiding it elsewhere.

@aloftus23 @stewartl97

(The reports are only visible if you select a fed agency in the organization dropdown).

@Matthew-Grayson
Copy link
Contributor

We spoke about it in the meeting today. It looks like it isn't being used on either site at the moment. The consensus was commenting out the code until we add reports functionality.

@Matthew-Grayson Matthew-Grayson dismissed their stale review September 18, 2023 18:28

Frontend Pipeline / test failure

@epicfaace
Copy link
Contributor

We spoke about it in the meeting today. It looks like it isn't being used on either site at the moment. The consensus was commenting out the code until we add reports functionality.

Yes, in that case, let's comment it out. Good to comment it out as we might need to re-add it in the future.

frontend/src/components/Header.tsx Outdated Show resolved Hide resolved
@Matthew-Grayson
Copy link
Contributor

I fixed the linting errors and replaced the header snapshot (the snapshot caused failure of the "Header matches snapshot" test. Additionally I fixed a type error related to the Organization interface used for testing by adding pendingDomains to testOrganization.

@courtneyc1 courtneyc1 merged commit ef2c654 into master Sep 20, 2023
10 checks passed
@courtneyc1 courtneyc1 deleted the 2237-hide-reports-page-1 branch September 20, 2023 20:12
@courtneyc1
Copy link
Contributor Author

updated typo/ commit from Ashwin's suggestion. Updated the branch since master had some changes since then. All changes approve and branch has no conflicts. Merged to master.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Frontend ASM-VDB User Interface
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Hide Reports Page
4 participants