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

Create accordion.html.twig #103

Merged
merged 11 commits into from
Sep 8, 2022
Merged

Conversation

wucherpfennig
Copy link
Contributor

@wucherpfennig wucherpfennig commented Jul 5, 2022

Description

Add accordion component. see #99

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist

  • I updated the documentation (see here)
  • I agree that this code will be published under the MIT license

@wucherpfennig
Copy link
Contributor Author

I did not expect this to happen... ;-)

I assume this is because of the added lock files from the last commit.
I think it is up to you to either "fix" the lock file or you decide to continue with php > 7.4 @kevinpapst

same goes for the other PR I suggested #104

@kevinpapst
Copy link
Owner

Nah. My mistake. I committed composer.lock which was created with PHP 8.1 - I'll check that tomorrow

@kevinpapst
Copy link
Owner

Sorry for that, can you please merge master into your branches

@wucherpfennig
Copy link
Contributor Author

Done

@cavasinf cavasinf added Feature Feature requested Status: Needs Review Not under investigation labels Jul 25, 2022
@kevinpapst
Copy link
Owner

And one last sorry for forgetting to come back to this PR.
What about a short docs page and I will add it then to the example app?

@wucherpfennig
Copy link
Contributor Author

Sorry for the long delay. I have added the required docs.

@wucherpfennig
Copy link
Contributor Author

Updated docs for this PR too.

@cavasinf
Copy link
Collaborator

I was about to do it, need that component 👍

docs/components-accordion.md Outdated Show resolved Hide resolved
docs/components-accordion.md Outdated Show resolved Hide resolved
templates/components/accordion.html.twig Outdated Show resolved Hide resolved
@kevinpapst
Copy link
Owner

@cavasinf the best review is using the component, could you give it a try already?

wucherpfennig and others added 2 commits August 23, 2022 22:27
Co-authored-by: Kevin Papst <kevinpapst@users.noreply.github.com>
Co-authored-by: Kevin Papst <kevinpapst@users.noreply.github.com>
Copy link
Collaborator

@cavasinf cavasinf left a comment

Choose a reason for hiding this comment

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

Tweaking+ Reformatting the doc

templates/components/accordion.html.twig Outdated Show resolved Hide resolved
templates/components/accordion.html.twig Outdated Show resolved Hide resolved
templates/components/accordion.html.twig Outdated Show resolved Hide resolved
<button class="accordion-button {% if loop.first == false %}collapsed{% endif %}"
type="button" data-bs-toggle="collapse" data-bs-target="#{{ _id_collapse }}"
aria-expanded="true">
{% if _raw == true %}{{ item.title | raw }}{% else %}{{ item.title }}{% endif %}
Copy link
Collaborator

Choose a reason for hiding this comment

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

@wucherpfennig what's happening if there is no title with the item ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did not check for that as a accordion without a title is kind of useless ;-)

we could "normalize" the title in order to have always some content (either the content or some "dummy title")

docs/components-accordion.md Outdated Show resolved Hide resolved
{% set _item_body_extraClass = item.options.body_extraClass ?? '' %}
<div class="accordion-item {{ _item_extraClass }}">
<div class="accordion-header {{ _item_title_extraClass }}" id="{{ _id }}-heading-{{ loop.index }}">
<button class="accordion-button {% if loop.first == false %}collapsed{% endif %}"
Copy link
Collaborator

@cavasinf cavasinf Aug 24, 2022

Choose a reason for hiding this comment

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

is item open should be an item option, not if not first loop.

Examples:

  • I want to have the second accordion item to be opened when arriving in the page.
  • Or none of them

@cavasinf
Copy link
Collaborator

@kevinpapst I'm in holidays RN, only thing I can do is "check by eye" and test it next Monday.

@cavasinf cavasinf added Status: Needs Work Need to be reworked and removed Status: Needs Review Not under investigation labels Aug 24, 2022
wucherpfennig and others added 2 commits August 24, 2022 10:13
Co-authored-by: CavasinF <cavasinf.info@gmail.com>
Co-authored-by: CavasinF <cavasinf.info@gmail.com>
@kevinpapst kevinpapst marked this pull request as draft August 31, 2022 13:11
@kevinpapst
Copy link
Owner

I did some minor updates, especially changing underscore to camelcase and reformatting of the markdown table and examples

@cavasinf
Copy link
Collaborator

cavasinf commented Sep 1, 2022

I've already suggested a DOC update, but it is outdated now 😢
But it was almost like yours now 👍

@kevinpapst
Copy link
Owner

I've seen that (thanks 😄 ) - but it was impossible to read, as it was so long and Github rendered it awfully, no chance to understand changes.

@wucherpfennig
Copy link
Contributor Author

Hey guys what do I need to do to help you? (Writing better code in the first place is not an option 😉)

@kevinpapst
Copy link
Owner

kevinpapst commented Sep 1, 2022

@wucherpfennig you could update to use my latest changes and then check the comments by @cavasinf

I haven't done a demo implementation yet, I will do that soon and provide feedback afterwards.

@wucherpfennig wucherpfennig marked this pull request as ready for review September 1, 2022 16:57
Copy link
Contributor Author

@wucherpfennig wucherpfennig left a comment

Choose a reason for hiding this comment

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

looks good

Copy link
Contributor Author

@wucherpfennig wucherpfennig left a comment

Choose a reason for hiding this comment

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

looks good

@wucherpfennig
Copy link
Contributor Author

Oh come on... tbh I do not know what else I should do ;-)

@cavasinf
Copy link
Collaborator

cavasinf commented Sep 8, 2022

LGTM now 👍

Copy link
Owner

@kevinpapst kevinpapst left a comment

Choose a reason for hiding this comment

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

I'll merge, add a demo page and check if everything works as expected.
Thanks @wucherpfennig 😄

@kevinpapst kevinpapst merged commit b01a7a2 into kevinpapst:main Sep 8, 2022
@kevinpapst kevinpapst removed the Status: Needs Work Need to be reworked label Sep 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature Feature requested
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Component - Accordion TablerBundle card collapsible VS Tabler Accordion
3 participants