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

Display information about consumed/provided services #125

Merged
merged 5 commits into from
Dec 29, 2023

Conversation

savetheclocktower
Copy link
Contributor

Closes #124.

Found a few places where we're reflecting raw user input onto the page without encoding the HTML, so I went through and switched a lot of <%-s to <%=s when it's obvious that they don't need to render unencoded HTML.

@confused-Techie
Copy link
Member

@savetheclocktower Thanks a ton for putting this together!

Overall, I'd say this looks fantastic! The code itself looks great, and now just time to actually test it.

Although, I'd ask if there's a special reason the CSS lives within the template itself, rather than ./src/site.css as that's where the rest of our CSS is applied.

But again, thanks for putting this together!

@savetheclocktower
Copy link
Contributor Author

savetheclocktower commented Dec 27, 2023

Although, I'd ask if there's a special reason the CSS lives within the template itself, rather than ./src/site.css as that's where the rest of our CSS is applied.

No reason. Addressed!

@confused-Techie
Copy link
Member

Alright, so having had the chance to test this out, overall it's fantastic!

(I'll be repeating a bit of what was said on Discord, just to make things more trackable)

What this PR does really well is exactly what it set out to do. Display and convey information in a fantastic way, the hover on service showing what version of the service is in use, the hyperlink text on the services to link to other packages that work with that same service, is a stroke of genius. Also love that you already made sure to bake in support for showing both consumed and provided services on the same package, since it's not all that uncommon.

Overall this is a great piece of work.

We mentioned on Discord some issues with the margin on the new page element, and considerations on inline-block vs block on the list elements of the actual services, but overall those are my only concerns and once addressed, I think we should be good to get this one merged! Thanks for all the effort and keeping up with my continuing asks, and promise this is the last of them lol

Copy link
Member

@confused-Techie confused-Techie left a comment

Choose a reason for hiding this comment

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

Thanks for addressing my feedback, I've got nothing else to add onto this one, and it looks great! Thanks for all the hard work, lets get it merged!

@confused-Techie confused-Techie merged commit fb2b4d5 into pulsar-edit:main Dec 29, 2023
2 checks passed
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.

Expose more service metadata
2 participants