Conversation
…emplate-management into feature/CCM-15018-request-proof-link
| text: markdownList('ul', [ | ||
| 'the template ID or IDs', | ||
| 'the email address you want the proofs sent to', | ||
| 'yo,ur service name', |
There was a problem hiding this comment.
| 'yo,ur service name', | |
| 'your service name', |
There was a problem hiding this comment.
Good spot thank you!
|
|
||
| await expect(previewPage.editButton).toBeVisible(); | ||
|
|
||
| await expect(previewPage.testMessageBanner).toBeHidden(); |
There was a problem hiding this comment.
if we're no longer asserting its hidden, shouldn't we be asserting that its visible instead?
| this.summaryList = page.locator('dl.nhsuk-summary-list'); | ||
| this.testMessageBanner = page.getByTestId('test-message-banner'); | ||
| this.testMessageBannerLink = this.testMessageBanner.locator('a'); | ||
| this.messageBanner = page.getByTestId('message-banner'); |
There was a problem hiding this comment.
I'm tempted to say should we have two separate locators for the request a proof banner and the test message one? so we can make clear assertions which differentiate between the two?
what do you think?
| }, | ||
| { | ||
| type: 'text', | ||
| text: '[Request a proof using our online form (opens in a new tab)](#).', |
There was a problem hiding this comment.
might need some TODOs in here?
| content: | ||
| 'This is only a basic preview. [Request a proof](/templates/request-a-proof/{{templateId}}) to preview this template properly.', | ||
| overrides: { | ||
| a: { props: { target: '', rel: '' } }, |
There was a problem hiding this comment.
are you happy overriding it this way round vs removing the default and explicitly adding the _blank etc in overrides when we want to add it?
There was a problem hiding this comment.
I think it really depends on whatever is used more often. At the moment it looks like the behaviour to open a new tab is the default behaviour and this is the edge case.
| @@ -1218,6 +1290,13 @@ const previewDigitalTemplate = { | |||
| EMAIL: | |||
| 'This is only a basic preview. [Send a test email](/templates/send-test-email/{{templateId}}) to preview this message properly.', | |||
There was a problem hiding this comment.
if you go with what I suggest in the pw tests about having separate locators for each banner, you could override the testid like we've done elsewhere in here and set it differently for each banner type in content, rather than it being hard-coded in PreviewDigitalTemplate?
|
|
||
| await expect(previewPage.editButton).toBeVisible(); | ||
|
|
||
| await expect(previewPage.testMessageBanner).toBeHidden(); |
There was a problem hiding this comment.
same comment as in the other file
|
|
||
| await expect(previewPage.editButton).toBeVisible(); | ||
|
|
||
| await expect(previewPage.testMessageBanner).toBeHidden(); |
There was a problem hiding this comment.
same comment as in the other files
| content: testMessageBanner[template.templateType], | ||
| overrides: undefined, | ||
| } | ||
| : requestProofBanner; |
There was a problem hiding this comment.
pretty sure the request proof banner only displays if routing is on, is that what you've done?
There was a problem hiding this comment.
no longer applies - agreed the banner is always displayed, its just the content that changes depending on digital proofing flag
4 participants
Description
Context
Type of changes
Checklist
Sensitive Information Declaration
To ensure the utmost confidentiality and protect your and others privacy, we kindly ask you to NOT including PII (Personal Identifiable Information) / PID (Personal Identifiable Data) or any other sensitive data in this PR (Pull Request) and the codebase changes. We will remove any PR that do contain any sensitive information. We really appreciate your cooperation in this matter.