Skip to content

Conversation

EliasMasche
Copy link
Contributor

Initial PR of the process for Scoped CSS

width: 125px;
}

img {
Copy link
Owner

Choose a reason for hiding this comment

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

This might be "dangerous" as it now targets every "img" whereas it was scoped to the component before. so probably you want something like actions-img or so?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Understood, gonna check that issue

Copy link
Owner

@linkdotnet linkdotnet left a comment

Choose a reason for hiding this comment

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

Currently the bUnit tests are red, as you "renamed" the CSS selectors

@EliasMasche
Copy link
Contributor Author

Currently the bUnit tests are red, as you "renamed" the CSS selectors

So I rename it back to the original or update the bUnit tests?

@linkdotnet
Copy link
Owner

Currently the bUnit tests are red, as you "renamed" the CSS selectors

So I rename it back to the original or update the bUnit tests?

I would do the latter

@EliasMasche
Copy link
Contributor Author

Removed the empty razor css files and added one that I missed.

An issue that remains is at home, the introduction hasn't show up yet of IntroductionCard.razor.css into home.css
and the CSS of PreviewImage.razor.css into components.css not being functional either

src/LinkDotNet.Blog.Web/wwwroot/css/home.css

firefox_hlO8DyJBDB

@linkdotnet
Copy link
Owner

Removed the empty razor css files and added one that I missed.

An issue that remains is at home, the introduction hasn't show up yet of IntroductionCard.razor.css into home.css and the CSS of PreviewImage.razor.css into components.css not being functional either

src/LinkDotNet.Blog.Web/wwwroot/css/home.css

firefox_hlO8DyJBDB

Hmm that might be because of --profile-background. Even though it should work. If you open your devtools and check the value of --profile-background is there something useful?

@linkdotnet
Copy link
Owner

There are some smaller tests that refer to the "old" structure:

 var items = cut.FindAll(".profile-keypoints li");

@EliasMasche
Copy link
Contributor Author

Removed the empty razor css files and added one that I missed.
An issue that remains is at home, the introduction hasn't show up yet of IntroductionCard.razor.css into home.css and the CSS of PreviewImage.razor.css into components.css not being functional either
src/LinkDotNet.Blog.Web/wwwroot/css/home.css
firefox_hlO8DyJBDB

Hmm that might be because of --profile-background. Even though it should work. If you open your devtools and check the value of --profile-background is there something useful?

Based on dev tools the profiles images are loaded correctly but don't show up on the page

vtqOu6cq6C

How it works: -profile-picture, -profile-image, .introduction-container, .introduction-background. This is my first time seeing this, and I am super new to dealing with CSS, I feel I am missing something.

Another bug is with PreviewImageRazor for blogposts that takes over the entire space and locks

firefox_xmNQ8yKUZh

The css is img that I renamed to comp-img the code is PreviewImage.razor in the HTML code uses the img tag how to rename properly or refactor for taking into account the proper CSS code

@if (string.IsNullOrEmpty(PreviewImageUrlFallback))
{
    <img src="@PreviewImageUrl" alt="Preview image blogpost" loading="@LazyLoadTag" decoding="@DecodingTag" />
}
else
{
    <picture>
        <source srcset="@PreviewImageUrl" type="@GetMimeType()"/>
        <img src="@PreviewImageUrlFallback" alt="Preview image blogpost" loading="@LazyLoadTag" decoding="@DecodingTag" />
    </picture>
}

@EliasMasche
Copy link
Contributor Author

There are some smaller tests that refer to the "old" structure:

 var items = cut.FindAll(".profile-keypoints li");

Do you need me to update them and search for other CSS renames too?

@EliasMasche
Copy link
Contributor Author

Another bug is with PreviewImageRazor for blogposts that takes over the entire space and locks

firefox_xmNQ8yKUZh

The css is img that I renamed to comp-img the code is PreviewImage.razor in the HTML code uses the img tag how to rename properly or refactor for taking into account the proper CSS code

@if (string.IsNullOrEmpty(PreviewImageUrlFallback))
{
    <img src="@PreviewImageUrl" alt="Preview image blogpost" loading="@LazyLoadTag" decoding="@DecodingTag" />
}
else
{
    <picture>
        <source srcset="@PreviewImageUrl" type="@GetMimeType()"/>
        <img src="@PreviewImageUrlFallback" alt="Preview image blogpost" loading="@LazyLoadTag" decoding="@DecodingTag" />
    </picture>
}

About this portion of the issue with PreviewImage.razor I did progress adding

.img-container {
	position: relative;
	width: 100%;
	height: 100%;
	overflow: hidden;
}

.comp-img {
	position: absolute;
	top: 0;
	left: 0;
	object-fit: cover;
	height: 100%;
	width: 100%;
}

For PreviewImageUrl works as before using <div class="img-container"> and class="comp-img"

    <div class="img-container">
        <img class="comp-img" src="@PreviewImageUrl" alt="Preview image blogpost" loading="@LazyLoadTag" decoding="@DecodingTag" /> 
    </div>

For PreviewImageUrlFallback is not working as in PreviewImageUrl.

    <div class="img-container">
        <picture>
            <source srcset="@PreviewImageUrl" type="@GetMimeType()" />
            <img class="comp-img" src="@PreviewImageUrlFallback" alt="Preview image blogpost" loading="@LazyLoadTag" decoding="@DecodingTag" />
        </picture>
    </div>

but for some reason now stopped working haha the PreviewImageUrl too

@linkdotnet
Copy link
Owner

There are some smaller tests that refer to the "old" structure:

 var items = cut.FindAll(".profile-keypoints li");

Do you need me to update them and search for other CSS renames too?

We have to make the tests work in one way or another. In this regard, the selector has to be aligned to the new "prefix" added.

@linkdotnet
Copy link
Owner

I have to checkout the branch later locally. I tried inside GitHub Codespaces and I got some 404 for the images. Not sure if this is because of GitHub Codespace or it showcases the underlying issue on why the images aren't showing up.

@linkdotnet
Copy link
Owner

@EliasMasche I took the opportunity to push to your PR. Hope you don't mind.

@EliasMasche
Copy link
Contributor Author

@EliasMasche I took the opportunity to push to your PR. Hope you don't mind.

Yeah, no problem. the only issue that remains is the home/introduction CSS, right?

@linkdotnet
Copy link
Owner

@EliasMasche I took the opportunity to push to your PR. Hope you don't mind.

Yeah, no problem. the only issue that remains is the home/introduction CSS, right?

Technically, yes. So of course everything should work as before, but overall I would like to go a step further, otherwise we just moved the CSS from one file to another.

Ideally:

  1. We can refactor many of the custom CSS classes to bootstrap 5 utility classes
  2. Consolidate the rest. There is much overlap in the content of those classes which can be moved together.
  3. Have then only one basic.css (well and the icon.css) to reduce the initial browser load.

@linkdotnet
Copy link
Owner

Just a small heads-up, for me the profile picture loads just fine, but the ShortBlogPost is a bit "different":

image

@linkdotnet
Copy link
Owner

The navbar items seem a bit closer than they were before and lost their "bold" font.

@linkdotnet
Copy link
Owner

Should also be fixed. Now the interesting part starts: Consolidating :D
That is where I see the real value in the PR and ticket

@EliasMasche
Copy link
Contributor Author

Roger that.

I checked your commits. I pulled into my local branch, and the CSS works now, but I am getting a 404 for the images.
Reason: strict-origin-when-cross-origin

firefox_obM70UTEbw

@EliasMasche I took the opportunity to push to your PR. Hope you don't mind.

Yeah, no problem. the only issue that remains is the home/introduction CSS, right?

Technically, yes. So of course everything should work as before, but overall I would like to go a step further, otherwise we just moved the CSS from one file to another.

Ideally:

1. We can refactor many of the custom CSS classes to bootstrap 5 utility classes

2. Consolidate the rest. There is much overlap in the content of those classes which can be moved together.

3. Have then only one `basic.css` (well and the `icon.css`) to reduce the initial browser load.

The end goal is to put all CSS into basics.css, refactor and reduce the custom CSS classes with bootstrap 5 utility classes.

That is where I see the real value in the PR and ticket

Good thing this time I put the PR as draft as it is more of a proposal and WIP.

@linkdotnet
Copy link
Owner

Interesting. By any chance, do you have a chrome based browser to cross-check if it happens there as well?
If so, if you go to "my website": steven-giesel.com - do you see the image? The difference is that the image on my website is hosted on a Blob storage and not as part of the application.

Good thing this time I put the PR as draft as it is more of a proposal and WIP.

No worries - that is part of the process. Issues/PR's are async in nature and therefore are more likely to have misunderstandings and whatnot.

@EliasMasche
Copy link
Contributor Author

Yeah, same issue coming from Edge Browser,

What it dislikes is that it comes from the same application but a different domain or locally triggering CORS if I use it from another website/domain, or blob storage works fine.

@EliasMasche
Copy link
Contributor Author

Hello there, sorry ( but may not be sorry) for the disappearance. I got relocated to a new team, a new project with an impossible deadline, so I wasn't able to work.

So now I pushed a new commit, redoing the rename of a class of CSS as it wasn't necessary, I believed they had the same name, I put them all inside basics.css

A question about the bootstrap, this sentence: We can refactor many of the custom CSS classes to Bootstrap 5 utility classes

Where can I find more information, or is it referring to the internal CSS classes of Bootstrap 5

@linkdotnet
Copy link
Owner

Hey hey - don't worry. OSS has to make space for other priorities so I totally get that and there is no need to apologize.

Where can I find more information, or is it referring to the internal CSS classes of Bootstrap 5

Sometimes I added stuff like padding: 4px or so. Which "should" be bootstrap utility classes like "p-1" (see: https://getbootstrap.com/docs/5.0/utilities/spacing/)

Or grid/flex layout can be solely described via Bootstrap's utility classes. There was often no need to create custom CSS.

@EliasMasche
Copy link
Contributor Author

I started making some changes to Bootstrap. What is the plan?

Besides, in this commit based on bootstrap, there are two ways to keep the custom class or use an inline. Checking through the repo, this is the only instance, so I guess using an inline CSS is fine
10e8472 (#413)

And I am already triggering some fails in tests because the HTML classes are missing in the razor files. For the moment, I am working on replacing them with Bootstrap but keeping the custom classes in basics.css for fallback. I will compare if the appearance is working as well as the old one, and once I am done, I will remove those custom classes.

@linkdotnet
Copy link
Owner

I started making some changes to Bootstrap. What is the plan?

I am not sure if I understand correct? What do you mean by that?

Besides, in this commit based on bootstrap, there are two ways to keep the custom class or use an inline. Checking through the repo, this is the only instance, so I guess using an inline CSS is fine
10e8472 (#413

If we keep stuff then still as classes. That is fine. If we see a chance to remove the custom CSS, that would be great, if not - then let's keep it as it is.

For the moment, I am working on replacing them with Bootstrap but keeping the custom classes in basics.css for fallback

Why would we need them as fallback? Sure, the tests have to be adopted and that is unfortunately a bit of a shortcoming on the current test-setup that it uses too much the concrete CSS classes. But that is a whole different story :D so for now it might be the best, to change failing ones to the new classes to it still targets the same object.

@EliasMasche
Copy link
Contributor Author

I am not sure if I understand correct? What do you mean by that?

You replied this question in the second, I was asking or trying to ask if I should refactor until it is done or not possible to do it.

Why would we need them as fallback? Sure, the tests have to be adopted and that is unfortunately a bit of a shortcoming on the current test-setup that it uses too much the concrete CSS classes. But that is a whole different story :D so for now it might be the best, to change failing ones to the new classes to it still targets the same object.

Sorry, what I mean is I still keep the old custom CSS classes for the time being for testing if the new HTML bootstrap CSS classes are working properly. Once I finish, I will do the cleaning. I am keeping a list of the custom CSS classes and commenting as I progress with the refactor

@EliasMasche
Copy link
Contributor Author

I did some progress overall, trying to match or mimic the appearance from custom CSS classes with Bootstrap 5 and with that able to reduce LoS in basic.css, the .blog-card class still not able to achieve progress or at least fully replace it but did at minor places, the rest took time.

There is this new warning:

Error: /Users/runner/work/Blog/Blog/src/LinkDotNet.Blog.Web/Features/Home/Components/IntroductionCard.razor(42,20): error S1144: Remove the unused private property 'IntroductionStyle'. (https://rules.sonarsource.com/csharp/RSPEC-1144) [/Users/runner/work/Blog/Blog/src/LinkDotNet.Blog.Web/LinkDotNet.Blog.Web.csproj]

  • Is it free to remove safely without any collateral damage?
  • Is it fine that I check the rest of the HTML classes based on Bootstrap 4 and update them to 5? I think I found some that are using the semantics of 4

About the tests, how is gonna be the progress going to be?

@linkdotnet
Copy link
Owner

Sorry for the late answer - I try to find some time next week for this and give you a proper answer to your questions

@EliasMasche
Copy link
Contributor Author

No problemo, as you once wrote, there is time for open-source, sometimes must side step for other priorities.

At least I know still alive the process ^^

@EliasMasche EliasMasche marked this pull request as ready for review September 10, 2025 17:27
@linkdotnet
Copy link
Owner

Okay - I would prefer if we rollback the changes then.
Also there seems to be a merge conflict from a different feature (multiple blog authors). Afterwards I'll have a final look and we can get that bad boy into master!

@EliasMasche
Copy link
Contributor Author

Reverted to the razor.css file. Regarding the merge conflict, I leave it in your hands, or I will check.

@linkdotnet
Copy link
Owner

I tried, but don't have the permission to push to your master branch:

To https://github.com/EliasMasche/Blog.git
 ! [remote rejected]   eliasmaster -> eliasmaster (permission denied)
error: failed to push some refs to 'https://github.com/EliasMasche/Blog.git'

The easy fix is to take the version from linkdotnet/master

@linkdotnet
Copy link
Owner

I would prefer if there is no inline-style at all: style="...".
I assume all of them can be either directly be replaced by bootstrap classes or at least be pushed into a css file.

}
<li class="read-time me-4">@BlogPost.ReadingTimeInMinutes minute read</li>
<li class="me-4">
<span class="me-1" style="font-family: 'icons'; font-weight: 900;">&#xe94f;</span>
Copy link
Owner

Choose a reason for hiding this comment

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

Here I would take the version from "my master" state.

@EliasMasche
Copy link
Contributor Author

EliasMasche commented Sep 16, 2025

I merged the conflict based on the changes and verified the inline styles= to proper Bootstrap classes. It seems I triggered some failed tests during the refactor of inline to bootstrap.

I checked and found differences at the logs of Tests failed compared to my local environment including different failed tests.

rider64_62oqonBpmX

@EliasMasche
Copy link
Contributor Author

Hey @linkdotnet, I've been checking the errors from the tests and have not been able to find the cause of the failed tests with SmokeTests.cs in lines:

https://github.com/linkdotnet/Blog/actions/runs/17752762473/job/50450353066?pr=413#step:6:30
Method ShouldBootUpApplication

https://github.com/linkdotnet/Blog/actions/runs/17752762473/job/50450353066?pr=413#step:6:49
Method IndexPage_HasTitleFromConfiguration

Did something from my branch or config get loaded wrong? because that is what it seems.

@linkdotnet
Copy link
Owner

At an point I commuted a failing commit as we were doing some bUnit work. Maybe just merging the latest master should resolve all of your issues

@EliasMasche
Copy link
Contributor Author

EliasMasche commented Oct 2, 2025

Thanks for the detail, all tests return green now, and I forgot to merge to the latest, the issue of coding, doing stuff at night 😅

@EliasMasche
Copy link
Contributor Author

All tests passed, all green, it seems ready for the last step when you are ready. On the last commits, I searched for all styles= and replaced them with Bootstrap classes or custom CSS.

@EliasMasche
Copy link
Contributor Author

Hey, @linkdotnet, asking if something else is missing for merging, and if not, do I start the merge process, or leave it to you

@linkdotnet
Copy link
Owner

Not really - sorry I was on vacation and will have a look tonight. The only blocker would be

  1. inline styles
  2. Way Different look and feel than before

@EliasMasche
Copy link
Contributor Author

Roger that, thanks for the info, waiting for the progress if passed or skipped for something. That went well, your vacation

@linkdotnet linkdotnet requested a review from Copilot October 13, 2025 16:57
<DonationSection />
</div>
}
<footer class="position-absolute bottom-0 w-100 text-center" style="height: 2.5rem;">
Copy link
Owner

Choose a reason for hiding this comment

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

Is this style here needed?

Copy link
Owner

Choose a reason for hiding this comment

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

If so - can this be moved "back" to the css

Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR refactors the CSS architecture by moving component-specific styles from individual .razor.css files into a centralized global CSS file. It also includes several modernization improvements using Bootstrap utility classes and updated styling patterns.

  • Consolidates scoped CSS styles into global basic.css file with organized region comments
  • Modernizes HTML/CSS by replacing inline styles with Bootstrap utility classes
  • Updates form styling and component layouts for better consistency

Reviewed Changes

Copilot reviewed 34 out of 37 changed files in this pull request and generated 5 comments.

File Description
src/LinkDotNet.Blog.Web/wwwroot/css/basic.css Added consolidated CSS styles from various components with region-based organization
Various .razor.css files Removed component-specific CSS files (empty after moving to global CSS)
Various .razor files Updated to use Bootstrap utility classes and modernized styling approaches

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

}

.skill-table td:first-of-type {
width:10%
Copy link

Copilot AI Oct 13, 2025

Choose a reason for hiding this comment

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

Missing semicolon at end of CSS property declaration.

Suggested change
width:10%
width:10%;

Copilot uses AI. Check for mistakes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed the missing colon.

Comment on lines 322 to 341
/*#region IntroductionCard.razor */
.introduction-background {
background-repeat: no-repeat;
background-size: cover;
background-image: linear-gradient(0deg, rgba(0, 0, 0, 0.4), rgba(0, 0, 0, 0.4)), var(--profile-background);
}

.profile-picture {
grid-area: profile-picture;
width: 175px;
height: 175px;
border-radius: 50%;
border: white;
margin: auto;
box-shadow: 0 0 0 4px white;
background-repeat: no-repeat;
background-position: center center;
background-size: cover;
}
/*#endregion */
Copy link

Copilot AI Oct 13, 2025

Choose a reason for hiding this comment

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

Duplicate CSS rules for IntroductionCard.razor component. The .introduction-background and .profile-picture classes are already defined in lines 206-244. This duplication should be removed to avoid conflicts and maintain consistency.

Copilot uses AI. Check for mistakes.

Copy link
Owner

Choose a reason for hiding this comment

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

Is this a false positive? If not, then we should remove it in the razor components css (or wherever the dup is)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, it was a good find on this one. I already removed in this commit 7870143 (#413)

<td>@shortCode.Name</td>
<td>
<div style="max-height: 100px; overflow-y: auto;">
<div class="overflow-auto mh-100px">
Copy link

Copilot AI Oct 13, 2025

Choose a reason for hiding this comment

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

The CSS class mh-100px appears to be a custom utility class that may not exist in Bootstrap. Consider using Bootstrap's standard max-height utilities like mh-100 or define this custom class in your CSS.

Suggested change
<div class="overflow-auto mh-100px">
<div class="overflow-auto" style="max-height: 100px;">

Copilot uses AI. Check for mistakes.

Copy link
Owner

Choose a reason for hiding this comment

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

Good point of Copilot, but no inline style :D

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The interesting detail that mh-100 does exist:
https://getbootstrap.com/docs/4.3/utilities/sizing/

but mh-100px is kinda doubtful, so 50/50 maybe, well I would give less pts as at the end recommend an inline style?

Copy link
Owner

Choose a reason for hiding this comment

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

Yeah: mh-100 means min-height:100%

Copy link
Owner

Choose a reason for hiding this comment

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

I would move it either to the basic.css or the razor.css of that component

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved to basics.css
bb15669 (#413)

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