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

Feature/refactor shared util layout #49

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

Conversation

mloeffle
Copy link
Member

As agreed in one of our JFs, here is a suggestion to reduce the content of the Layout/Shared/Util folders.

  • Layout got removed
  • Util got cleaned up
  • Shared got cleaned up

My thoughts about this were:

  • Own folder: Everything with a specific content get into a folder to group components which make the most sense to be used together
  • Shared: Components offering a blueprint that does get its content eg from Slots an can be used in every context
    • still in one folder so we have one place to look what is available
  • compoments root:
    • components that cant be assigned with others to be combined in a dedicated folder
  • Util
    • components that don't offer customiziation in each context and just get placed somewhere (eg UtilityPill, UtilityLogo, they get only one to very few props)
    • Disclaimer: I wanted these components to go into components root, but the linting then fails due to vue/multi-word-component-names so I wanted to open the conversation to change this

Copy link
Contributor

Choose a reason for hiding this comment

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

This looks good for now. Just wanted to note here that we will have to see how well the Pagination can be adapted to multiple different contexts. If any issues arise in the future there might be a debate to move it to Shared, as it grows more complex.

Copy link
Member Author

Choose a reason for hiding this comment

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

Agree, we have to see how its going to work out.

Copy link
Contributor

Choose a reason for hiding this comment

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

This (alongside LandingPage and NavigationPage) feels kinda weird. Putting them in a folder does make sense, but the naming "Frontend" is a little bland. Perhaps something like "PageProvider" would make it more clear what they do? 🤔 so they would be called -> PageProviderDetail, PageProviderLanding and PageProviderNavigation.

Copy link
Member Author

Choose a reason for hiding this comment

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

I understand what you mean, but sadly the logic on how the content is rendered requires them to start with Frontend since the component name is generated from the SW route like frontend.detail.page.

@mloeffle mloeffle requested a review from m-borgmann September 24, 2024 18:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants