Skip to content

Conversation

@kevwhitt-hee
Copy link
Contributor

@kevwhitt-hee kevwhitt-hee commented Jun 19, 2025

PR Template

TODO:

Test it here too
https://technologyenhancedlearning.github.io/GitPageBlazorWASM-TestGHPage/
check version matches branch name and pullrequest release

  • maybe a rule that any introduced css for example if bringing in an enum. needs to be refactored to nhse-tel? so tel-nhse is source of truth on all things css

TODO: About

  • SO FAR no js controllers are not going to be covered in testing in the package it will be done by how used in LH
    But in future we could simulate them with just an api with stock response or find a programatic way, or build a hosted environment.
    The gh-page are pure wasm so not supported for nojs

  • brave check no js (Locally not on wasm static test page)

TODO: This is just replicating LH for now ... ideas

  • Link to replicated component
  • Link design confluence if coming from design
  • Tick box for JS
  • If testers find an issue create an automated test of what they did, even potentially with playright record test functionality
  • something about is there a linked lh branch using the change

JIRA link

TD-5671

Description

Updates readme setup instructions, including:

  • Making variable names consistent
  • Proofing and improving instructions
  • Tweaking according to my experience of the setup process

Screenshots

Paste screenshots for all views created or changed: mobile, tablet and desktop, wave analyser showing no errors.


Developer checks

(Leave tasks unticked if they haven't been appropriate for your ticket.)

I have:

  • Run the IDE auto formatter on all files I’ve worked on and made sure there are no IDE errors relating to them
  • Written or updated tests for the changes (accessibility ui tests for views, tests for controller, data services, services, view models created or modified) and made sure all tests are passing
  • Manually tested my work with and without JavaScript (adding notes where functionality requires JavaScript)
  • Tested any Views or partials created or changed with Wave Chrome plugin. Addressed any valid accessibility issues and documented any invalid errors
  • Updated my Jira ticket with testing notes, including information about other parts of the system that were touched as part of the MR and need to be tested to ensure nothing is broken
  • Scanned over my pull request in GitHub and addressed any warnings from the GitHub Build and Test checks in the GitHub PR ‘Files Changed’ tab
    Either:
  • Documented my work in Confluence, updating any business rules applied or modified. Updated GitHub readme/documentation for the repository if appropriate. List of documentation links added/changed:
  • Confirmed that none of the work that I have undertaken requires any updates to documentation

@kevwhitt-hee
Copy link
Contributor Author

@Phil-NHS, this is my draft PR as promised. I'll carry on working through and add more updates.

@kevwhitt-hee
Copy link
Contributor Author

@Phil-NHS. An observation about adding the package source: Doing this through the VS interface using the raw values may be easier than using PowerShell or editing appdata files directly and will make it universal to all solutions.

@kevwhitt-hee
Copy link
Contributor Author

@Phil-NHS, I seem to be going through a lot of tweaking appsettings and env variables for purposes that aren't altogether clear when I go through the setup process. For example, I am told to set TELBlazorPackageSource 5 or 6 times, often to the same value. I think we need to simplify this and just give the reader a single setup process in the readme, perhaps explaining the other options and why you might use them later on. I move from one section to the next at the moment, not much changes and I get a sense of déjà vu. I have tried to tidy the first few instances of this in this PR, but I'm getting a bit lost tbh.

@kevwhitt-hee
Copy link
Contributor Author

I seem to get a build error, and I don't know why. I have started trying to follow the branch and commit naming rules, but I must admit, they feel very fussy. Is there a definite benefit from having them? I think devs will struggle with them.

@kevwhitt-hee
Copy link
Contributor Author

@Phil-NHS it mentions adding appsetting.Development.json to various projects in the solution. I can find appsettings.Development.json.template only in the TELBlazor.Components.ShowCase.E2ETests.WasmServerHost project. Should I be copying this to the other projects and renaming as appsetting.Development.json?

@Phil-NHS
Copy link
Collaborator

@Phil-NHS, I seem to be going through a lot of tweaking appsettings and env variables for purposes that aren't altogether clear when I go through the setup process. For example, I am told to set TELBlazorPackageSource 5 or 6 times, often to the same value. I think we need to simplify this and just give the reader a single setup process in the readme, perhaps explaining the other options and why you might use them later on. I move from one section to the next at the moment, not much changes and I get a sense of déjà vu. I have tried to tidy the first few instances of this in this PR, but I'm getting a bit lost tbh.

You are right. The reason for each setup is so you can see the project build using the project reference, local package reference, and git package reference.

The reason the settings can be the same is if someone gets stuck they can know how they all should be. so maybe a code block for the settings would be better and just say which has changed.

The setup instead of my intention of enabling the person setting up to check each thing works as they go is making them have an experience of each option. This is not what setup instructions are for and i shouldnt have done it this way. If this were desired it should be in a separate section suggest as "Developers Project Configuration Options".

I think the change is at most have a separate section to the setup with option for how you can configure the project relating to how it consumes the TELBlazor.Components package/project and in the setup just have the do the easiest which is use the project reference and not require them to make or consume any packages.

As you've said quick setup instructions would be the right first section. But this setup should still just get them straight from A to B just with a little more info for if they get stuck.

@Phil-NHS
Copy link
Collaborator

@Phil-NHS it mentions adding appsetting.Development.json to various projects in the solution. I can find appsettings.Development.json.template only in the TELBlazor.Components.ShowCase.E2ETests.WasmServerHost project. Should I be copying this to the other projects and renaming as appsetting.Development.json?

No are you using the most recent release they should be in all of them ... though i think i did notice one missing so at least one i did add after you started i think. ... just checked the repo there are several but i will make sure all of them are there

@Phil-NHS
Copy link
Collaborator

Phil-NHS commented Jun 19, 2025

I seem to get a build error, and I don't know why. I have started trying to follow the branch and commit naming rules, but I must admit, they feel very fussy. Is there a definite benefit from having them? I think devs will struggle with them.

the branch rules i think can be used with the git workplace, but really because there wasnt jira tickets with thought out names and preexsiting branches to copy i thought it might just encourage me to keep to good naming, and to reflect the commit naming. I can remove the checks from the pipeline and from the project i am happy with or without them.

The commit names however are important, the pipeline reads the names and generates a version number and change log based off them so we should keep to them and then the repo, and package, will be version and have a changelog so we can see what changed between package versions.

@Phil-NHS
Copy link
Collaborator

I seem to get a build error, and I don't know why. I have started trying to follow the branch and commit naming rules, but I must admit, they feel very fussy. Is there a definite benefit from having them? I think devs will struggle with them.

i'd need to see the build error, if its because the package is missing build the blazor.component project first, and/or change to project reference. or check that nuget is looking for it in the right place. but id need to see it

@Phil-NHS
Copy link
Collaborator

@Phil-NHS. An observation about adding the package source: Doing this through the VS interface using the raw values may be easier than using PowerShell or editing appdata files directly and will make it universal to all solutions.

i think you are right, and because of the caching, i think actually having a local package source and git one in nuget and it just finding the one it can makes sense. ive found despite making the right changes sometimes it doesnt look in the right place. The environmental values need to be there for the pipeline but locally yeah i should just add nuget set up instructions through the ui, and its familiar. which is a bonus.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Seems much better. Though from you comments on the ticket i think a rewrite is required and better approaches to some of the bits.

@kevwhitt-hee
Copy link
Contributor Author

I seem to get a build error, and I don't know why. I have started trying to follow the branch and commit naming rules, but I must admit, they feel very fussy. Is there a definite benefit from having them? I think devs will struggle with them.

i'd need to see the build error, if its because the package is missing build the blazor.component project first, and/or change to project reference. or check that nuget is looking for it in the right place. but id need to see it

The build error was in .net build but seems to have gone away after a subsequent commit.

@kevwhitt-hee
Copy link
Contributor Author

@Phil-NHS, let me know if you want me to continue with this and make more changes. If not, is it worth merging what I've done so far and working on some of the agreed suggestions?

@Phil-NHS Phil-NHS marked this pull request as ready for review June 23, 2025 09:21
@Phil-NHS Phil-NHS merged commit c9f3b45 into master Jun 23, 2025
15 checks passed
@kevwhitt-hee
Copy link
Contributor Author

🎉 This PR is included in version 1.9.0-feat-button-for-cicd-testing.1 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

@kevwhitt-hee
Copy link
Contributor Author

🎉 This PR is included in version 1.9.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants