Skip to content

Fixed SiteBranding CSS bug for generated sites #220

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

Merged
merged 2 commits into from
May 8, 2023

Conversation

qtzar
Copy link
Contributor

@qtzar qtzar commented May 3, 2023

When using the 'generate' command the generation of the site-branding.css file was occurring before the workspace DSL had been read in to memory meaning that it was always using the defaults instead of any properties in the DSL.

I've moved the call to 'copySiteWideAssets' to after the DSL is read in for both the local workspace DSL and for repository based DSLs. In the case of a repository based DSL the copySiteWideAssets call is only done once on the default branch.

Fixes #219

@jp7677
Copy link
Contributor

jp7677 commented May 3, 2023

The copySiteWideAssets method only copies static content. Moving this call further down really shouldn’t make a difference.

The generating of the branding stylesheet happens later in https://github.com/avisi-cloud/structurizr-site-generatr/blob/main/src/main/kotlin/nl/avisi/structurizr/site/generatr/site/SiteGenerator.kt#L65

I think what’s missing is a line like https://github.com/avisi-cloud/structurizr-site-generatr/blob/main/src/main/kotlin/nl/avisi/structurizr/site/generatr/site/SiteGenerator.kt#L108 in generateStyle and we need to ensure that the path to the branding stylesheet is still correct.
Since adding the current branch to exportDir already happens in multiple places and we need to add one more, we should also refactor this to improve readability.

(I’m currently on vacation so I can’t actually test right now)

@jp7677 jp7677 closed this May 3, 2023
@jp7677 jp7677 reopened this May 3, 2023
@jp7677 jp7677 self-requested a review May 3, 2023 19:18
Copy link
Contributor

@jp7677 jp7677 left a comment

Choose a reason for hiding this comment

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

See comment above.

@qtzar
Copy link
Contributor Author

qtzar commented May 3, 2023

oh I think you are right and I did a bad test on my side.

I'm seeing the issue because I build three branches and the last branch that I build does not have any custom style on the dsl so I get the defaults and that is overwriting the site-branding.css file in the root and being applied to all sites.

I'm going to redo this PR and rewrite it to move the site-branding.css to the branch directory and have it loaded from there depending on which branch is being viewed.

Copy link
Contributor

@jp7677 jp7677 left a comment

Choose a reason for hiding this comment

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

See comments.

Do you dare to drop the first two commits with git interactive rebase and git push force? This would keep the commits less cluttered for this PR.

@qtzar qtzar force-pushed the SiteBrandingFix branch from 27b7df3 to 218cf6d Compare May 4, 2023 13:00
@qtzar
Copy link
Contributor Author

qtzar commented May 4, 2023

@jp7677 I've made the changed on the comments you left but I've noticed something unusual in addition to the fix above.

When you generate a site ( or local serve ) there is a 'puml' and 'svg' directory created in the root folder with names of the files duplicated in the filename, for example if you have a softwareSystem called MySystem there is a file created called MySystem-SystemContext-MySystem-SystemContext.puml while in the branch folders puml folder there is a similar file called MySystem-SystemContext.puml

I'm wondering if this is a bug of some sort. It seems to happen during the generateHtmlFiles stage of the process.

@jp7677
Copy link
Contributor

jp7677 commented May 4, 2023

@jp7677 I've made the changed on the comments you left but I've noticed something unusual in addition to the fix above.

Thanks a lot, looks good and cool that you were able to drop the first two commits.

When you generate a site ( or local serve ) there is a 'puml' and 'svg' directory created in the root folder with names of the files duplicated in the filename, for example if you have a softwareSystem called MySystem there is a file created called MySystem-SystemContext-MySystem-SystemContext.puml while in the branch folders puml folder there is a similar file called MySystem-SystemContext.puml

I'm wondering if this is a bug of some sort. It seems to happen during the generateHtmlFiles stage of the process.

This is the working directory for creating the embedded/clickable diagrams (https://github.com/avisi-cloud/structurizr-site-generatr/blob/main/src/main/kotlin/nl/avisi/structurizr/site/generatr/site/DiagramGenerator.kt#L37). Here we always pass the root exportDir as location. As far as I’m seeing this is fine, since we also check the definition of the diagrams when deciding if they are up to date or not. As a side effect this greatly improves performance by not repeatedly generating the same diagrams across multiple branches.

@jp7677
Copy link
Contributor

jp7677 commented May 4, 2023

LGTM, I’ll do a brief test when being again in front of my machine (unless @dirkgroot beats me to it), though I expect this PR to be fine and will merge afterwards.

@jp7677 jp7677 merged commit 1774c9c into avisi-cloud:main May 8, 2023
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.

Site branding not applied in SiteGenerator from repository
2 participants