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

Component organisation #2

Closed
2 tasks done
peteryates opened this issue May 5, 2020 · 4 comments · Fixed by #5
Closed
2 tasks done

Component organisation #2

peteryates opened this issue May 5, 2020 · 4 comments · Fixed by #5

Comments

@peteryates
Copy link
Member

peteryates commented May 5, 2020

Currently (as of #1) the two example components are separated by directory

app/components
├── panel
│   ├── panel_component.html.erb
│   └── panel_component.rb
└── start_now_button
    ├── start_now_button_component.html.erb
    └── start_now_button_component.rb

And invoked like this:

StartNowButton::StartNowButtonComponent.new(text: text, href: href)

This is ugly and repetitive. It would be nicer if they were namespaced under GOVUK and in a flatter structure:

GOVUK::StartNowButtonComponent.new(text: text, href: href)

Questions

  • Do we need to name them all <thing>Component? Is the common suffix just adding noise?
  • Are people ok with not separating components by directory? We'll probably have quite a few components and having them - the ruby files and templates - all in a single directory might get a bit unwieldy
@tvararu
Copy link
Collaborator

tvararu commented May 5, 2020

I like separating components by directory; I'd argue for colocating (unit) specs next to components as well.

👍 for GOVUK::StartNowButtonComponent

@tijmenb
Copy link
Contributor

tijmenb commented May 5, 2020

I think GOVUK might be too generic, when I was on GOV.UK we tried to avoid it because of the risk of collisions with internal & external libraries (compounded by problems with Govuk vs GOVUK and Rails inflection).

To the suffix question, I think -Component is indeed noise if we have a namespace, so perhaps this works:

GovukComponent::Panel.new
GovukComponent::Tag.new
GovukComponent::StartNowButton.new

peteryates added a commit that referenced this issue May 5, 2020
This change has the disadvantage of placing multiple components in the
same directory but the benefit of a more sensible set of naming
conventions

Refs #2
@peteryates
Copy link
Member Author

I've submitted #5 and it feels like the the right approach. I can't see a way of keeping the names simple and predictable and organising with one component per directory.

Leaving the task unchecked in case I've missed something obvious.

@peteryates
Copy link
Member Author

Merged #5, I like this approach apart from the previously stated reasons (multiple components in one directory, the weird capitalisation of Govuk 😞).

Everything else is better though, so until something better comes along let's go with it

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants