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

fix: adjust content based on testing feedback #88

Merged
merged 4 commits into from
Dec 12, 2023

Conversation

jessicamcinchak
Copy link
Member

@jessicamcinchak jessicamcinchak commented Dec 4, 2023

Feedback coming out of theopensystemslab/planx-new#2529

My first draft of this data dictionary was a bit too optimistic about which "Select the works" old flows and new triage flows are actually in use. I've gone through a number of example submissions and other flows now and filled out all possible project types. I think this should be more comprehensive now 🤞

@jessicamcinchak jessicamcinchak marked this pull request as draft December 4, 2023 15:47
'extend.front': 'Add a front extension',
'extend.outbuilding':
Copy link
Member Author

@jessicamcinchak jessicamcinchak Dec 7, 2023

Choose a reason for hiding this comment

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

Services currently have mixed use of extend.outbuilding singular and extend.outbuildings plural (among other similar examples, but this one seems to be most widespread as it affects granular children).

It's less than ideal to include both, but I'd rather be able to remove project types later when service content is tidied up than fail to validate all possible journeys through live services now.

Copy link
Contributor

Choose a reason for hiding this comment

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

Agreed that whilst we're below v1 this is the most pragmatic approach.

Removing any fields would be a breaking change in the schema but I think at this stage that's actually relatively insignificant as we'd be the ones affected (and the old ones wouldn't get used).

It might be worth looking into how we can decorate these field as deprecated before removing them - possibly overkill at this stage though if it proves tricky tbh!

@jessicamcinchak jessicamcinchak marked this pull request as ready for review December 11, 2023 14:28
@jessicamcinchak jessicamcinchak requested a review from a team December 11, 2023 14:28
Copy link
Contributor

@DafyddLlyr DafyddLlyr left a comment

Choose a reason for hiding this comment

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

A higher number of new additions than I was expecting - good work! 🕵️‍♀️

@@ -73,13 +85,14 @@ export const ProjectTypes = {
'alter.openings.add.door.side':
'Add new doorways to the side of the building',
'alter.openings.add.window': 'Add one or more new windows',
Copy link
Contributor

Choose a reason for hiding this comment

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

This one is still window not windows?

Copy link
Contributor

Choose a reason for hiding this comment

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

Just seen comment below about remaining singular and plural instances 👍

Copy link
Member Author

Choose a reason for hiding this comment

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

This example is actually different than the singular/plural "duplicates" like "outbuilding" below - from what I found in content, it's actually always singular window in the parent case, and then children switch to plural windows 💫

door is always singular, so this was easy to confuse ! I'd love to do a big find&replace exercise to clean these up soon, and really pay attention to these subtle details to better align values in this particular enum.

'extend.front': 'Add a front extension',
'extend.outbuilding':
Copy link
Contributor

Choose a reason for hiding this comment

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

Agreed that whilst we're below v1 this is the most pragmatic approach.

Removing any fields would be a breaking change in the schema but I think at this stage that's actually relatively insignificant as we'd be the ones affected (and the old ones wouldn't get used).

It might be worth looking into how we can decorate these field as deprecated before removing them - possibly overkill at this stage though if it proves tricky tbh!

@jessicamcinchak jessicamcinchak merged commit 0b68f5a into main Dec 12, 2023
@jessicamcinchak jessicamcinchak deleted the jess/content-fixes branch December 12, 2023 09:45
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