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

224 create new table component #2166

Draft
wants to merge 16 commits into
base: main
Choose a base branch
from

Conversation

davidtrussler
Copy link
Contributor

@davidtrussler davidtrussler commented May 13, 2024

Trello

This is a first iteration of the new table component for the Publisher landing page. It’s still a work in progress but at a useful stage for an initial review. It still needs some custom styling but seems to work pretty much as expected. There are currently no other components associated with this repo so this creates a new component guide section which can be viewed locally at this URL: http://publisher.dev.gov.uk/component-guide/table

I had a few concerns emerge in my mind as I was working on it, as detailed below. Apart from that the code is just good enough to make it work and will need some tidying plus I’d need to add a Jasmine test for the JS.

Concerns referred to are:

  • Is the creation of a new component really the best way to achieve what is required here? To my mind the use of a component would suggest that we are introducing a reusable pattern but during its development this component has become ever more tightly tied to the specific requirement such that it would never be used for anything else.
  • The actual implementation here rolls together a number of different existing components (table, accordion and summary list) and sprinkles some custom stuff on top to make it work as required. This feels a little bit inelegant but might not be a huge issue.
  • I had some concerns about the accessibility of the component though I haven’t looked into that fully yet. Mostly this is just a feeling that the structure is a little bit overly complex.
How it looks at the minute
Screenshot 2024-05-14 at 10 16 13

@mtaylorgds
Copy link
Contributor

I agree that this does seem very specific to the one use-case, in which case we should probably make that clear from the naming.

I think it makes some sense to have the code for this pulled out from the page itself (to avoid the page being massive), but maybe just using a partial would suffice?

I can't comment so much on the accessibility aspects (maybe we can ask about that on Slack), but it does at least seem usable via keyboard.

@davidtrussler davidtrussler force-pushed the 224_Create-new-table-component branch 3 times, most recently from b0dc379 to 00aa922 Compare May 24, 2024 15:58
@davidtrussler davidtrussler force-pushed the 224_Create-new-table-component branch 3 times, most recently from 0967537 to 5537dec Compare May 31, 2024 11:13
@davidtrussler davidtrussler force-pushed the 224_Create-new-table-component branch 3 times, most recently from 84e2b5e to e20fa49 Compare June 3, 2024 15:20
@davidtrussler davidtrussler force-pushed the 224_Create-new-table-component branch from e20fa49 to 627477d Compare June 4, 2024 11:56
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