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

Exportable nav #1072

Merged
merged 14 commits into from
Aug 8, 2024
Merged

Exportable nav #1072

merged 14 commits into from
Aug 8, 2024

Conversation

hanbyul-here
Copy link
Collaborator

@hanbyul-here hanbyul-here commented Jul 25, 2024

Related Ticket: #1071

Description of Changes

Exporting nav from UI side so it can be used to override its menu from GHG side.
The code changes look like a lot, but most of them are just pieces moving around and making the components accept props.

This change checks the veda.config to see if there are any defined navItems and use it if there is one instead of the default one. There can be 4 types of nav item, which are internalLink, externalLink, dropdown, modal. The details of each type will be documented.

Notes & Questions About Changes

The current version does not mark the dropdown item as 'active' when its child is selected.

Validation / Testing

{Update with info on what can be manually validated in the Deploy Preview link for example "Validate style updates to selection modal do NOT affect cards"}

Instance pr: US-GHG-Center/veda-config-ghg#457

  • remove test config from veda.config
  • Add documentation : will do it after style polish

Copy link

netlify bot commented Jul 25, 2024

Deploy Preview for veda-ui ready!

Name Link
🔨 Latest commit 6e2223d
🔍 Latest deploy log https://app.netlify.com/sites/veda-ui/deploys/66b3cfd21145270008d9de9c
😎 Deploy Preview https://deploy-preview-1072--veda-ui.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@hanbyul-here hanbyul-here marked this pull request as ready for review August 2, 2024 18:59
Copy link
Collaborator

@dzole0311 dzole0311 left a comment

Choose a reason for hiding this comment

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

Had some time and had a first look into the PR. Apologies if there are things you're still planning to change or modify.

app/scripts/components/common/google-form.tsx Show resolved Hide resolved
as: 'span',
size: 'small'
})`
/* styled-component */
Copy link
Collaborator

Choose a reason for hiding this comment

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

Probably a leftover comment

app/scripts/components/common/page-header/index.tsx Outdated Show resolved Hide resolved
@dzole0311
Copy link
Collaborator

I noticed a strange vertical line on the right of the dropdown menu + the border-radius does not seem applied on the top right and bottom right corners of the dropdown. Is this as expected?

Screenshot 2024-08-08 at 15 05 14

@hanbyul-here
Copy link
Collaborator Author

It is not really expected, but it is a style problem that we could not resolve from the past tech-debt: #691 😅 I will try to handle it in the next PR.

@dzole0311
Copy link
Collaborator

It is not really expected, but it is a style problem that we could not resolve from the past tech-debt: #691 😅 I will try to handle it in the next PR.

Ah, I think it's just the scrollbar showing if I'm not mistaken. Adding overflow-y: auto; on the DropContent would probably fix it, but let's address & test it in the next PR to avoid regressions in other places where it's used.

@hanbyul-here hanbyul-here merged commit 76b03c4 into main Aug 8, 2024
8 checks passed
@hanbyul-here hanbyul-here deleted the exportable-nav branch August 8, 2024 13:19
sandrahoang686 added a commit that referenced this pull request Aug 13, 2024
## 🎉 Features
- Add isHidden prop for optional hiding of stories
#1090,
#1100
- Add Exportable nav with configurable dropdowns
#1072

## 🚀 Improvements
- Date picker aligns with temporal density & simplify timeline header
#1086

## 🐛 Fixes
- 🦗
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.

2 participants