Skip to content

Conversation

@SteveSandersonMS
Copy link
Member

@SteveSandersonMS SteveSandersonMS commented Sep 14, 2020

I noticed this while doing some other test updates. In case you're wondering why we don't have test coverage that caught this, we actually do, here, but looks like those template baseline tests aren't running.

Description

Not sure how it happened, perhaps a bad rebase, but at some point before #25613 was merged, one of the changes in it got lost. The wwwroot/styles.css file was meant to have been moved to Component1.razor.css (and a comment in it was meant to have been removed due to it not really serving a purpose).

This definitely was the case at some point as I remember verifying it. However it's clearly not there in the final PR that was merged. Update: root-cause explanation in later comment.

Customer Impact

Unless fixed, the RCL template won't use scoped CSS as clearly intended, and it will make no sense why.

Developers consuming RCLs will have to take extra steps to use them (i.e., add <link> tags pointing to their styles).

The styling in the RCL won't be scoped as intended.

Regression?

No

Risk

No. As long as the broader CSS isolation feature works correctly (which we are already counting on), then using that feature here presents no extra risk.

@ghost ghost added the area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates label Sep 14, 2020
@SteveSandersonMS SteveSandersonMS requested a review from a team September 14, 2020 13:50
@SteveSandersonMS SteveSandersonMS added this to the 5.0.0-rc2 milestone Sep 14, 2020
@SteveSandersonMS
Copy link
Member Author

Update: I've tracked down how this happened. Via git reflog, I see that I did do a manual rebase using cherry-pick and misunderstood the git cherry-pick <hash1>...<hash2> syntax. It only includes the changes after hash1, and hence I lost the first commit in the range I intended to pick.

Copy link
Contributor

@mkArtakMSFT mkArtakMSFT 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 tracking this down, Steve!

@SteveSandersonMS
Copy link
Member Author

What's the approval process for RC2 now, @mkArtakMSFT? Does @Pilchie still approve or does this need to go through tactics now?

@Pilchie Pilchie added the Servicing-approved Shiproom has approved the issue label Sep 14, 2020
@Pilchie
Copy link
Member

Pilchie commented Sep 14, 2020

I'll let this one squeak by, but from now on we'll go through Tactics.

@Pilchie
Copy link
Member

Pilchie commented Sep 14, 2020

Approved for .NET 5 RC2.

@Pilchie Pilchie closed this Sep 14, 2020
@Pilchie Pilchie reopened this Sep 14, 2020
@Pilchie
Copy link
Member

Pilchie commented Sep 14, 2020

Sorry for the close/re-open :( This did pass CI before, so we could skip it this time if we wanted to.

@mkArtakMSFT mkArtakMSFT merged commit ccbc46c into release/5.0-rc2 Sep 14, 2020
@mkArtakMSFT mkArtakMSFT deleted the stevesa/fix-rcl-template branch September 14, 2020 16:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates Servicing-approved Shiproom has approved the issue

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants