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

EES-5544 Change data set import page accordions into a table #5459

Open
wants to merge 7 commits into
base: dev
Choose a base branch
from

Conversation

bennettstuart
Copy link
Collaborator

@bennettstuart bennettstuart commented Dec 12, 2024

@bennettstuart bennettstuart marked this pull request as ready for review December 12, 2024 13:32
@bennettstuart bennettstuart marked this pull request as draft December 12, 2024 16:19
@bennettstuart bennettstuart marked this pull request as ready for review December 20, 2024 14:42
@bennettstuart bennettstuart force-pushed the EES-5544 branch 2 times, most recently from 9a80962 to 3024245 Compare January 13, 2025 10:19
tests/robot-tests/tests/admin/bau/upload_files.robot Outdated Show resolved Hide resolved
tests/robot-tests/tests/libs/admin-common.robot Outdated Show resolved Hide resolved
@@ -23,9 +23,10 @@ user checks table row heading contains
user waits until element contains xpath://table/tbody/tr[${row}]/th[${column}] ${expected}

user checks table cell contains
[Arguments] ${row} ${column} ${expected} ${parent}=css:table
[Arguments] ${row} ${column} ${expected} ${parent}=css:table ${wait}=%{WAIT_SMALL}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does this need a wait? Typically, we could use some other keyword to check that the table (or something in it) has rendered before checking the table cells.

Most 'check' keywords don't include waits for this reason, so this is fairly inconsistent.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Was added due to table updating asynchronously to the form submit, but have abstracted the waits into separate keywords

tests/robot-tests/tests/libs/tables-common.robot Outdated Show resolved Hide resolved
@bennettstuart bennettstuart requested a review from ntsim January 20, 2025 15:59

a,
button {
margin-bottom: 1rem;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Seems like the spacing is a bit excessive now:

image

Additionally, we should be using govuk-spacing function for this.

1 spacing unit:

image

2 spacing units:

image

Feels like 1 spacing unit might actually be fine

Comment on lines +90 to +92
<th scope="col">Size</th>
<th scope="col">Status</th>
<th scope="col">Actions</th>
Copy link
Collaborator

@ntsim ntsim Jan 22, 2025

Choose a reason for hiding this comment

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

Seems like we're missing a bunch of columns that appear in the ticket and acceptance criteria? Is this on purpose? If so, the ticket needs to be updated to describe precisely what is needed.

If not, looks like we need to add those in. We'll need to update the tests accordingly as well!

Copy link
Collaborator Author

@bennettstuart bennettstuart Jan 22, 2025

Choose a reason for hiding this comment

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

With the addition of the "View details" modal, we've opted to remove some columns to make the rows more concise at first glance wit the added option to see more information. Cam and I have discussed briefly here in the ticket comments in which I showed example:
image

And he responded:

For the columns to show, I think I’d drop the uploaded by, and make the status wrap for data replacement in progress so that it’s over two lines, I think both of those together should give enough space for longer subject titles?

Leaving just:

  • Title, Size, Status and Actions

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