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

Hds::Icon Groundwork #2198

Merged
merged 8 commits into from
Jul 23, 2024
Merged

Hds::Icon Groundwork #2198

merged 8 commits into from
Jul 23, 2024

Conversation

zamoore
Copy link
Contributor

@zamoore zamoore commented Jun 28, 2024

📌 Summary

This PR is part of #2207

If merged, this PR will:

  • Install @hashicorp/flight-icons as a direct dependency of components
  • Port the load-sprite instance initialization functionality over from ember-flight-icons to components
  • Ensure that the load-sprite initializer only runs once. (In existing and new instance initializers)

🔗 External links

Feature branch PR: 2207
Jira ticket: HDS-3515


Copy link

vercel bot commented Jun 28, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
hds-showcase ✅ Ready (Inspect) Visit Preview Jul 23, 2024 7:49pm
hds-website ✅ Ready (Inspect) Visit Preview Jul 23, 2024 7:49pm

yarn.lock Outdated Show resolved Hide resolved
@Dhaulagiri
Copy link
Collaborator

Dhaulagiri commented Jul 3, 2024

It doesn't look like the logic for avoiding double insertion of the sprite is working as I see it twice in the website 👀

Screenshot 2024-07-03 at 09 24 39

I'd have to look at it or think on it more, but could be related to it being rendered via prember/fastboot?

@zamoore
Copy link
Contributor Author

zamoore commented Jul 3, 2024

@Dhaulagiri I'm unable to reproduce the double loading locally. Maybe something environment related?

@didoo
Copy link
Contributor

didoo commented Jul 4, 2024

@zamoore see the deployed preview website: https://hds-website-git-hdsicon-importicons-hashicorp.vercel.app/

screenshot_3919

@zamoore
Copy link
Contributor Author

zamoore commented Jul 5, 2024

@didoo @Dhaulagiri - I had to make the same change to addon-main that I made to the load-sprite initializer for cases where that isn't used. Checked the preview now and it does appear to only load once.

Copy link
Contributor

@didoo didoo left a comment

Choose a reason for hiding this comment

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

LGTM

(let's wait for @Dhaulagiri or @alex-ju to approve too, for this one)

@didoo didoo requested review from alex-ju and Dhaulagiri July 8, 2024 14:51
Copy link
Member

@alex-ju alex-ju left a comment

Choose a reason for hiding this comment

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

Code looks good! I'm happy for us to get it in, assuming we tested this in a product as well.

Comment on lines +13 to +14
!config.emberFlightIcons?.lazyEmbed &&
!config.__flightIconsSpriteLoaded &&
Copy link
Member

Choose a reason for hiding this comment

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

note: I assume the double underscore notation is used to denote the 'internal' nature of this property. across the codebase we have all sorts of notations for this, from __PROPERTY__ to _property and __property that would ideally get aligned in the future

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What do you think the best way to document this would be?

@didoo
Copy link
Contributor

didoo commented Jul 19, 2024

@zamoore do you think we should do a quick check in Cloud UI using this specific branch, to make sure we're not missing anything? unless you're 100% confident is not necessary to retest what happens if we deploy only this groundwork first

I'll leave it to you to make the call :)

@zamoore
Copy link
Contributor Author

zamoore commented Jul 22, 2024

@didoo I'd rather be more cautious than less. I'm going to run a quick test this morning to verify.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants