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

Reference link and icon #66

Merged
merged 3 commits into from
Jan 8, 2025
Merged

Reference link and icon #66

merged 3 commits into from
Jan 8, 2025

Conversation

tmushayahama
Copy link
Contributor

@tmushayahama tmushayahama commented Jan 7, 2025

Issues

Changed

  • Fixed link to reference url
  • added newspaper icon, so far it is image

Note

  • we might need to truncate number of evidences shown

@pkalita-lbl, I tried couple of ways mentioned here #62, but no success. Feather Icons were close, but couldn't find the right icon. So I have temporarily added the png image instead. But would rather use svg (in case later styling needed). So if you know some ways, I have included the svg image, check if you can replace the png. Also, the the icon height is hard-coded, idk if a variable height is needed

cc @vanaukenk @kltm

@pkalita-lbl
Copy link
Collaborator

I added a change which inlines the SVG provided by https://icons.getbootstrap.com/icons/newspaper/ into the wc-genes-panel component. This type of direct embedding is an entirely blessed way to use Bootstrap icons. Does that look okay to you?

@tmushayahama
Copy link
Contributor Author

Yes, this is awesome and super clean. I really like how it avoids importing the entire icon library, focusing instead on the exact SVGs we need. For later, if needed in more places, would it make sense to create a global web component for svg rendering or global function? This could help abstract and reuse the rendering.

Copy link
Collaborator

@pkalita-lbl pkalita-lbl left a comment

Choose a reason for hiding this comment

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

Yep, if the icon is needed in other places in the future we could look at hoisting the function up to a place where it can be shared by multiple components. But no need to overly abstract things at this point, IMO.

@tmushayahama tmushayahama merged commit 59fb78e into main Jan 8, 2025
1 check passed
@tmushayahama tmushayahama deleted the refactor branch January 8, 2025 22:48
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