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

Announcement component using USWDS #1031

Merged
merged 25 commits into from
Jul 26, 2024
Merged

Announcement component using USWDS #1031

merged 25 commits into from
Jul 26, 2024

Conversation

hanbyul-here
Copy link
Collaborator

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

I made banner as a part of veda config: https://github.com/NASA-IMPACT/veda-ui/pull/1031/files#diff-828262a6a39b2c45c85b5fcdf1b4615ff5d3d69f662078fd27aa9846fb29f36fR20 (Compare this configuration to the current setup: https://github.com/US-GHG-Center/veda-config-ghg/blob/develop/veda.config.js#L45-L49)

I structured the component with the ongoing refactoring in mind. But let me know if you see any problem 🙇

I also re-introduced purge-css because I wanted to use utility classes from uswds. This comes with some prices ex. manual safelist and no hot reloading. I left a comment in the code, I am open to taking a different approach,

  • Add a documentation about configuring Banner

Copy link

netlify bot commented Jul 2, 2024

Deploy Preview for veda-ui ready!

Name Link
🔨 Latest commit 737efff
🔍 Latest deploy log https://app.netlify.com/sites/veda-ui/deploys/66a3b13e2347610008934a46
😎 Deploy Preview https://deploy-preview-1031--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 changed the title [do not merge] Announcement component using USWDS Announcement component using USWDS Jul 19, 2024
@hanbyul-here hanbyul-here marked this pull request as ready for review July 22, 2024 15:53
app/scripts/main.tsx Outdated Show resolved Hide resolved
@@ -4,5 +4,15 @@ module.exports = {
plugins: [
require('autoprefixer'),
require('postcss-import'),
require('@fullhuman/postcss-purgecss')({
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The styles doesn't get hot-reloaded unfortunately :[ (I have to restart the dev server if I want to include a new class name etc.)

<div>
{isOpen &&
(<div className='position-relative'>
<USWDSBanner aria-label={appTitle} className={type !== infoTypeFlag? 'bg-secondary-lighter': ''}>
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Floagging that this component uses uswds default color theme now: https://designsystem.digital.gov/design-tokens/color/theme-tokens/

I don't want to drag this PR with another configuration setup especially considering there hasn't been a request for a different type of banner. (But then it might be better to just get rid of this special styling for now?)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Until needed I'm in support of the default theme 👍

@dzole0311
Copy link
Collaborator

@hanbyul-here could you add steps on how to configure the banner to test this PR? Thanks! 🙇

@sandrahoang686
Copy link
Collaborator

sandrahoang686 commented Jul 23, 2024

Hmm this is more of a thought pattern of how we may want to use USWDS components within our codebase. My thought is it might be a good pattern to create a wrapper for all USWDS components we use in our codebase and then forward props... this way it creates a pattern where it can be easily decoupled from breaking changes that the DS could introduce later on in the future (this may or may not happen but we'd be easily prepared)... and then just also it creates a decoupling layer in general. If there are any changes that happen to the DS component, it would also isolate us to just having a make a change in one place (the wrapper) instead of throughout our codebase, if that makes sense.

What this would look like is having a Design System directory for USWDS with our wrappers around these components. Then other components in our codebase would reference our internal directory. common/banner/index.tsx isn't too complicated and is already pretty close to this pattern since it doesn't add much functionality on top, so its mostly already a simple wrapper and you are already referencing it that way.

What do you think?

@hanbyul-here
Copy link
Collaborator Author

@dzole0311 No wonder you couldn't see the banner—I configured it wrong 😓 . You should be able to see it now. I also added a document about adding it through the configuration file.

@sandrahoang686 I never had experience with the pattern that you suggested. I tried to educate myself a bit, and some people like the pattern because of the pros you mentioned, and others caution that it may introduce additional complexity and maintenance requirements. It is difficult for me to form an opinion on this issue without hands-on experience. Since we are changing our design system, do you mind giving me a concrete example of how we could have benefited from the pattern if we used it for the devseed ui library?

@sandrahoang686
Copy link
Collaborator

sandrahoang686 commented Jul 25, 2024

@hanbyul-here sure, you already are implementing the pattern actually 😅. What you did was wrap the DS USWDSBanner element with our own custom wrapper without applying much additional complex logic on top of it, a simple enough wrapper. Our components or layout-root (here) references this wrapper instead of directly importing the USWDSBanner element. This created a nice abstraction layer where if there are breaking changes to the DS then we can just manage it in our wrappers creating isolation. For example, if DS rolls out breaking changes to some of their ui elements - like banner is deprecated (random example), we just have to update our wrapper file with a new banner component instead of touching every file in our codebase that imports from uswds directly. Another example is when props change..

I'm just thinking though, does it make sense to create a directory /design-system/components where these wrappers can live instead so its more explicit and we keep to this pattern, never importing from uswds directly in our own components. Its cleaner, creates a nice easy to manage abstraction layer, is simpler and more organized, and is quick to decouple. There isn't much overhead risks or complexity, I think it actually mitigates it.

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.

Looks great, thanks for adding the first USWDS component in the Dashboard 🥳

To follow up on our discussion from the other day, I'm in favor of a simple structure like the one used in the PR, but I’m also open to the idea of nesting the components in an additional subdirectory (as @sandrahoang686 suggested). It’s a minor adjustment, so I'll leave the decision to you.

@hanbyul-here
Copy link
Collaborator Author

I made the directory of common/uswds and made the most simple wrapper for uswds components there: https://github.com/NASA-IMPACT/veda-ui/blob/30f5475f02937adfa79c13d802b2e6f6c5746428/app/scripts/components/common/uswds/banner.tsx

While doing it, I was really not sure if this is the way you pictured it, @sandrahoang686. I will merge this PR so as not to spill the scope, but please feel free to make follow-up edits if you see any improvement.

@hanbyul-here
Copy link
Collaborator Author

@dzole0311 @sandrahoang686 Another note that I made purgecss not to kick in when NODE_ENV is development - I am aware that this might have a problem that what you see on development is not what you get from the preview build. But purgecss makes dev server quite slow, so I decided to take this route for now. I am really open to changing this setup anytime with alternative approaches.

@sandrahoang686
Copy link
Collaborator

sandrahoang686 commented Jul 26, 2024

"simple structure" is opinion based as both are simple.. its just organization and points of reference in the code. When there is a directory we can follow that has all the USWDS or whatever DS elements with simple wrappers its easy to pinpoint, follow patterns, and quicker to decouple. While having it blended with other common component just doesn't provide that clear distinction and pattern standard. But most important pattern is there which is not using USWDS directly in our components 👍🏼 the organization and logic looks good to me

@hanbyul-here hanbyul-here merged commit fc9035d into main Jul 26, 2024
8 checks passed
@hanbyul-here hanbyul-here deleted the 288-announcement branch July 26, 2024 15:06
sandrahoang686 added a commit that referenced this pull request Jul 31, 2024
**HOLD until we merge #1073

## 🎉 Features
* Added Announcement component using USWDS Design System and adding
purge-css #1031

## 🚀 Improvements
* Add logic for tracking out-of-view Timeline playheads for E&A page
#1067
* Abstract Away Faux Modules from MapBlock Component and expose as
library component #1046

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

3 participants