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

chore: update to datagrid #935

Merged
merged 5 commits into from
Oct 13, 2023
Merged

chore: update to datagrid #935

merged 5 commits into from
Oct 13, 2023

Conversation

IanKrieger
Copy link
Member

@IanKrieger IanKrieger commented Oct 12, 2023

Resolves: #931


Screen Shot 2023-10-12 at 2 41 46 PM

@IanKrieger IanKrieger requested a review from a team October 12, 2023 18:35
Copy link
Collaborator

@tackley tackley left a comment

Choose a reason for hiding this comment

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

This is a great improvement on what was there. I think there's some more improvements that can be implemented, but am happy for this to come in later PRs:

  • ideally the data grid should show the number of rows that fits into the height available, especially on the campaign & creative lists. On ads-admin this needed a wider overhaul of how the overall screen is laid out, though.
  • on the campaign list, using a "checkbox" in a case where only single selection is allowed is odd - this should be a radio button or use data grid's row selection
  • ^ there's also an annoying delay between selecting a row and the buttons along the top being enabled / disabled, in the campaign list (I've observed this before, it isn't datagrid related)
  • ^ maybe an action cell is a better way of doing this, but that is a debatable
  • I'm a bit nervous about how usable this is on narrow screens - or on tablets / mobile
  • it'd be great if things like state used a dropdown in the filter list (I think this is just a matter of setting singleSelect type and providing the list of values to the definition)

@IanKrieger IanKrieger merged commit 7f6b46e into master Oct 13, 2023
6 checks passed
@IanKrieger IanKrieger deleted the chore/update-to-datagrid branch October 13, 2023 15:27
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.

Switch campaign view to datagrid
2 participants