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

Feature/html formatting #22

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

Antaris
Copy link

@Antaris Antaris commented Feb 12, 2019

Hi,

Really digging this project, I've been using it the last couple of days for some internal project documentation. One thing I've found is that the HTML output can be a little hard to read, so for your consideration, I've built some tweaks that introduce a couple of changes:

  1. Use OS-default sans-serif fonts for non-code elements of the generated HTML
  2. Use OS-default monospace fonts for code elements of the generated HTML
  3. Basic syntax highlighting of types and keywords

Before:
omd-formatting-before

After:
omd-formatting-after

If this works, happy to have contributed, or if you're planning to do this at some point yourself, no worries, just thought it would be useful 👍

@Antaris
Copy link
Author

Antaris commented Feb 12, 2019

Ah, also should have looked beforehand, didn't realise this PR existed: #19, apologies @MarcosMeli

@dotMorten
Copy link
Owner

Thank you! I'm glad you find it useful.
Your PR has a lot of great improvements.
TBH I intended for the UI to exactly match the OMDs created by Visual Studio, so I had been a little reluctant to change the generated UI too much. However I do see the improvements you made are a good idea - I like the highlighting of keywords/types.
However I'm not convinced monospace makes it more readable - In my mind it's a less readable font, and it makes things look wider.
Also Segoe UI is a Windows-only font, so I'd prefer we default to fonts all platforms have to ensure consistency.

I'm also wondering if I should find a way for people to define their own stylesheet and override styles. I could just reference a stylesheet, and people can add the stylesheet if they desire, and if it's not there, it just falls back to the header-defined ones?

@@ -209,7 +209,7 @@ public void WriteType(INamedTypeSymbol type, INamedTypeSymbol oldType)
if (type.GetInterfaces(oldType).Any())
{
isEmpty = false;
sw.Write("<br/>Implements ");
sw.Write("<br/>Implements <span class='code'>");
Copy link
Owner

Choose a reason for hiding this comment

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

I'd rather we don't monospace it all.

@@ -261,7 +262,7 @@ private string GetIcon(ISymbol type, string content)
}
if (icon == "")
return content;
return $"<li class='{icon}'>{content}</li>";
return $"<li class='{icon} code'>{content}</li>";
Copy link
Owner

Choose a reason for hiding this comment

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

I think it's a good idea to create a style users can easily override, but not sure this should be the style name, or that it should default to a mono-space font.

.objectBox > .header.noMembers { border-radius: 10px; }
.objectBox > .header > span { font-size:20px; }
.objectBox > .header > span:not(.code) { font-size:20px; }
.code { font-family: "Consolas", "Courier New", "Courier", monospace; font-size: 14px; }
Copy link
Owner

Choose a reason for hiding this comment

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

I'd prefer a sans-serif style font here as default


.members li { background-repeat: no-repeat; padding-left:20px; background-position: left center; }
.members li { background-repeat: no-repeat; padding-left:20px; background-position: left 2px; }
Copy link
Owner

Choose a reason for hiding this comment

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

Nice catch on the vertical alignment! 👍

@Antaris
Copy link
Author

Antaris commented Feb 12, 2019

Amazing, will review these tomorrow and make some tweaks.

In terms of custom CSS, could be a command line arg that accepts a path to a stylesheet that is merged? We use BEM notation a lot in our CSS stuff, could be a good way to layout your component styles that people can easily override?

@dotMorten
Copy link
Owner

could be a command line arg that accepts a path to a stylesheet that is merged?

One thing to consider is that there are multiple formats - HTML is just one (and I hope to make it extensible in the future). So we'd need some sort of generator-specific way to send parameters if that's the way we want to go.
IMHO the simplest thing is to just include a reference to a stylesheet, and if it's there it is used and can override the default styles, otherwise it's just a swallowed 404.

@dotMorten dotMorten closed this Jun 19, 2020
@dotMorten dotMorten reopened this Jun 19, 2020
@dotMorten dotMorten changed the base branch from master to main June 19, 2020 15:50
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