Skip to content

Conversation

@DanBloxham-sw
Copy link
Contributor

Ran all unit tests. Tested in Chrome, Edge, Firefox, IE. Tested with screen reader, mobile view, and no JS.

Screen shots:
Desktop:
image

Mobile:

Desktop no JS:
image

Copy link
Contributor

@JBsoftwire JBsoftwire left a comment

Choose a reason for hiding this comment

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

I have one question, which may have an obvious answer. But I'm going to move this across anyway so we can potentially get it merged quickly.

Copy link
Contributor

@stellake stellake left a comment

Choose a reason for hiding this comment

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

Screenshots look great - just a few comments! ⭐

Comment on lines 61 to 62
<div class="nhsuk-grid-row">
<div class="nhsuk-grid-column-full">
Copy link
Contributor

Choose a reason for hiding this comment

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

Any reason we're adding new row/column elements for the "copy course" bit?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was to keep the Generate Email and Copy Course link on separate lines. Do we have an alternative preferred method for this?

Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't a button/p element on a new line automatically?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Normally, yes, but when the button was present, it had the js-only-inline class which put everything inline. I've changed it to js-only-block, and removed the divs, and it achieves the same effect.

</div>
<div class="nhsuk-grid-row">
<div class="nhsuk-grid-column-full">
<a href="#" class="js-only-inline copy-course-link nhsuk-link--no-visited-state" id="copy-course-@Model.CustomisationId">Copy course link</a>
Copy link
Contributor

Choose a reason for hiding this comment

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

Just wanted to double check if you've checked that this is the most appropriate HTML for this functionality (for clarity - I haven't looked into it at all)

Comment on lines 61 to 62
<div class="nhsuk-grid-row">
<div class="nhsuk-grid-column-full">
Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't a button/p element on a new line automatically?

Copy link
Contributor

@stellake stellake left a comment

Choose a reason for hiding this comment

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

Looks great! 👍

@stellake stellake merged commit ea46c76 into master Jul 22, 2021
@stellake stellake deleted the HEEDLS-431-CourseSetupPopulateCourseCards branch July 22, 2021 13:48
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.

3 participants