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

BccAccordion additions #304

Merged
merged 3 commits into from
May 21, 2024
Merged

BccAccordion additions #304

merged 3 commits into from
May 21, 2024

Conversation

StevenMalaihollo
Copy link
Contributor

@StevenMalaihollo StevenMalaihollo commented Apr 26, 2024

Restructuring

When trying to use the BccAccordion component in an Event product we found out that none of the variants fitted our needs. This caused us to rethink the approach variants.

Currently we have a mix of variants usage:

  • BccButton: has the variants primary, secondary and tertiary
  • BccTooltip: has the variants: dark, white and grey
  • BccAccordion used to have the variants: readonly, desktop, brand and interactive

Because the variant namings differs so much, they are hard and unexpected to use. Also, Variants that had a very specific design would often only work for 1 use case. In this PR we propose to repurpose the variant prop into the following:

"Variants should be a descriptive term for the overall appearance of the component, not a guideline for design practices (like primary, sec...) or a specific value (like dark, white, grey). It should instead be a term for a specific 'style' of the component that is reusable for different domains"

Examples of this new variants would be:

  • BccAccordion has the variants: filled, outlined, soft and ghost
  • BccTooltip has the variants: filled, outlined
  • BccButton has the variants: filled, outlined and ghost

These variants can then be further customized by adding a context like warning, neutral.

The plan is to agree on this approach in this PR for the BccAccordion. And then refactor the other components in a follow-up PR once agreement is reached.

The inspiration for this approach comes from Nuxt UI

Change summary

Allong with the variant restructuring, the following was also added to the BccAccordion

  1. Add open prop to initiate the Accordion in an expanded state
  2. Add info prop to add a text to the right of the title.

Change type

  • No review
  • Small PR
  • Big PR
  • Refactor

Copy link
Contributor

Azure Static Web Apps: Your stage site is ready! Visit it here: https://brave-coast-028fe6203-304.westeurope.3.azurestaticapps.net

Copy link
Contributor

github-actions bot commented May 3, 2024

Azure Static Web Apps: Your stage site is ready! Visit it here: https://brave-coast-028fe6203-304.westeurope.3.azurestaticapps.net

Copy link
Contributor

github-actions bot commented May 6, 2024

Azure Static Web Apps: Your stage site is ready! Visit it here: https://brave-coast-028fe6203-304.westeurope.3.azurestaticapps.net

@StevenMalaihollo StevenMalaihollo marked this pull request as ready for review May 6, 2024 10:57
Copy link
Collaborator

@laurensgroeneveld laurensgroeneveld left a comment

Choose a reason for hiding this comment

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

Code wise this looks fine to me. I do have two notes on this in general:

  1. This is a breaking change, so technically we would need a major release, but I think it's fine because I suspect nobody outside of Event is actually using this component. For renaming the other, existing components I think we do need a v3.0, so that should be considered a bit more carefully.
  2. On the repurposing of the variant prop:

"Variants should be a descriptive term for the overall appearance of the component, not a guideline for design practices (like primary, sec...) or a specific value (like dark, white, grey). It should instead be a term for a specific 'style' of the component that is reusable for different domains"

This sounds fine to me, but keep in mind the current naming very closely follows the naming of the design system in Figma. This extends to the color tokens:
image

I rather not have our implementation start to diverge from the naming the designers have, as to not create unneeded confusion. It's good to note though that the components that are created in Figma do have these different variants declared as well:

image

So perhaps it's not as big a change, but still we need to make sure to align this with the design team.

@u12206050
Copy link
Member

We have been speaking to the design team to align on this new approach. We have also decided that any restrictions to design are to be only applied at the design level in figma, such that components we create can be more customisable/configurable and more developer focused rather than being one to one to the design system setup in figma.
Visually it will still be the same, but it may be that our components support more colors than are currently allowed in BCC product designs.

The thought behind this is, too make it more useful across all products that are developed by us developers, even though not all products are BCC, eg. Work app.

Also, as most "proper" products will have a design in figma first, developers will just use that to implement what they see, even though the components themselves can do more.

@laurensgroeneveld
Copy link
Collaborator

Alright 👍 Originally we defined that the design library should not be too customizable as to prevent developers from implementing things that do not follow the design system. But it's fine to change our minds, just make sure everyone understands the implications.

The thought behind this is, too make it more useful across all products that are developed by us developers, even though not all products are BCC, eg. Work app.

That's what a "theme" would be for I feel. Currently everything is "bccForbundet" theme, with the idea being we could introduce a new theme that mostly follows the same guidelines but with different colors, fonts, etc.

@StevenMalaihollo StevenMalaihollo merged commit dfb9e7d into main May 21, 2024
7 checks passed
@StevenMalaihollo StevenMalaihollo deleted the accordion-improvements branch May 21, 2024 06:59
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.

3 participants