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

feat(spinner): Adding component - FRONT-3223 #2255

Merged
merged 5 commits into from
Dec 6, 2021
Merged

Conversation

planctus
Copy link
Contributor

@planctus planctus commented Nov 15, 2021

This is a twig implementation of a "spinner" component.
It is not clear how to make this usable in an application,

  1. if we have specific components to apply this to we could include this template, then the visibility of the spinner can be handled through the addition of a class.
  2. Otherwise this could be used as a "template" only for a functionality to be implemented at the application level
  3. We could potentially set the markup through js so that given a selector we could inject the spinner markup, but it doesn't look like an elegant nor a failsafe approach.

UPDATE:
After talking with Francesco it seems that this will ony be used as a example markup that needs to be copied into a drupal function, so not using any twig template.
Therefore the current approach seems potentially correct.

@github-actions
Copy link

github-actions bot commented Nov 15, 2021

Copy link
Contributor

@emeryro emeryro left a comment

Choose a reason for hiding this comment

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

General remarks:

  • the order of pages on the website is not correct
    image
  • the overlay in storybook is not visible for primary. Maybe we could have some dummy content somewhere to see the difference

- "size" (string) options:
[ small, medium, large ]
- "text" (string) (default: '')
- "visible" (boolean) (default false)
Copy link
Contributor

Choose a reason for hiding this comment

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

missing "centered" and "overlay" parameters

options: [ small, medium, large ]
- **"text"** (string) (default: '')
- **"visible"** (boolean) (default: false)
- **"centered"** (boolean) (default: true))
Copy link
Contributor

Choose a reason for hiding this comment

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

missing "overlay" parameter

{% endif %}

{% if _visible %}
{% set _css_classes = _css_classes ~ ' ecl-spinner--active' %}
Copy link
Contributor

Choose a reason for hiding this comment

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

as the parameter is called "visible", maybe we could use the same name for the class (".ecl-spinner--visible"). Same for the overlay

category: 'Content',
},
},
variant: {
Copy link
Contributor

Choose a reason for hiding this comment

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

when selecting negative spinner on storybook (in the controls), we should apply a background (similar to what has been done for the links), otherwise it is not visible (in primary story)
Or we just don't show this control

}
}
// stylelint-disable-next-line plugin/selector-bem-pattern
.path {
Copy link
Contributor

Choose a reason for hiding this comment

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

can't we apply the namespace/bem pattern for this css class too?

emeryro
emeryro previously approved these changes Nov 16, 2021
@emeryro emeryro merged commit 6690c89 into v3-dev Dec 6, 2021
@emeryro emeryro deleted the front-3223-spinner branch December 6, 2021 12:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants