- Issue created by @mherchel
- First commit to issue fork.
- last update
over 1 year ago Custom Commands Failed - @stevenspriggs opened merge request.
- Status changed to Needs review
over 1 year ago 9:03pm 7 June 2023 - Status changed to Needs work
over 1 year ago 10:38pm 7 June 2023 - ๐บ๐ธUnited States smustgrave
CC failure in the MR.
Tried testing though and after applying the MR and clearing cache I get this error
Twig\Error\LoaderError: Template "olivero:tags" is not defined in "core/themes/olivero/templates/field/field--node--field-tags.html.twig" at line 42. in Twig\Loader\ChainLoader->getCacheKey() (line 99 of vendor/twig/twig/src/Loader/ChainLoader.php).
- ๐จ๐ฆCanada Robert Ngo Montreal โ๏ธ
Robert Ngo โ made their first commit to this issueโs fork.
- last update
over 1 year ago Custom Commands Failed - @robert-ngo opened merge request.
- last update
over 1 year ago 29,426 pass, 6 fail - Assigned to Robert Ngo
- last update
over 1 year ago 29,427 pass, 5 fail - last update
over 1 year ago 29,427 pass, 5 fail - last update
over 1 year ago 29,427 pass, 5 fail - last update
over 1 year ago 29,427 pass, 5 fail - last update
over 1 year ago 29,427 pass, 5 fail - ๐บ๐ธUnited States mherchel Gainesville, FL, US
After a quick review, this is looking really good!
The only quick nit is that you can simplify your include function.
instead of doing a
{{ include('olivero:tags', { attributes: attributes, label_hidden: label_hidden,
you can simplify to
{{ include('olivero:tags', { attributes, label_hidden,
Also, I'm guessing that the tests are failing because SDC is not being enabled by default. So tests won't pass at this point
- last update
over 1 year ago Custom Commands Failed - last update
over 1 year ago 29,427 pass, 5 fail - last update
over 1 year ago 29,404 pass, 7 fail - ๐บ๐ธUnited States mherchel Gainesville, FL, US
This is looking just about perfect! The only major change is to assemble the classes and title_classes arrays within the
field--node--field-tags.html.twig
file.Note that tests will not pass on this because SDC is not a part of standard profile. Once this gets to a good place we'll postpone it until SDC becomes stable.
- last update
over 1 year ago 29,434 pass, 5 fail - Status changed to Needs review
over 1 year ago 7:30pm 14 June 2023 - Status changed to Postponed
over 1 year ago 11:19pm 14 June 2023 - ๐บ๐ธUnited States smustgrave
Changes look good (with my limited knowledge of SDC). I'm assuming the failures are due to SDC not being in standard. Postponing issue until that happens.
- Issue was unassigned.
- Status changed to Active
4 months ago 4:09pm 20 July 2024 - Status changed to Needs review
4 months ago 5:58pm 20 July 2024 - ๐ท๐ธSerbia finnsky
I started a new approach because I believe that styles in components should not be tied to Drupal. This makes them more flexible.
So it shouldn't be
.field .node-- and other thingsJust pure simple and clear css
Also fixed flexboxes and added support for label above
Please check!
- ๐ฎ๐ณIndia ehsann_95
The styles are looking as expected. Only thing i can figure out is the flex direction is column now. Attaching screenshot below
- ๐ท๐ธSerbia finnsky
Yeah. I've added above/inline label display support.
- ๐ฎ๐ณIndia ehsann_95
@finnsky, This makes sense and the above/inline field display is working as expected.
Inline:
Above:
- ๐บ๐ธUnited States smustgrave
smustgrave โ changed the visibility of the branch 3365389-convert-oliveros-tags to hidden.
- ๐บ๐ธUnited States smustgrave
smustgrave โ changed the visibility of the branch 3365389-sdc-olivero-tags to hidden.
- Status changed to Needs work
4 months ago 6:19pm 29 July 2024 - ๐บ๐ธUnited States smustgrave
If we are adding additional functionality believe the summary needs to be update to include this is more then just converting to SDC.
Also this changes the current design with the label no longer being inline, which may need sub-maintainer sign off for that design change.
Thanks.
- Status changed to Needs review
4 months ago 6:36pm 29 July 2024 - ๐ท๐ธSerbia finnsky
more then just converting to SDC
we should never just converting ;)
SDC give us chance to do it better.
- ๐บ๐ธUnited States smustgrave
Normally 100% agree but would say in this case think getting to SDC first and in follow up doing enhancement would be best, just my opinion, or both seem to get delayed.
- ๐ซ๐ทFrance pdureau Paris
Hi,
Looking at: https://git.drupalcode.org/project/drupal/-/merge_requests/8860/diffs
items: title: Tags Items type: array item: type: object properties: content: title: Tags Item type: object attributes: title: Item Attributes type: Drupal\Core\Template\Attribute
1. No need to have "Tags Items" as title, we know we are in a Tag component. "Items" is enough.
2. This is not JSON schema valid. You need
items
instead ofitem
:type: array items: type: object
Source: https://json-schema.org/understanding-json-schema/reference/array
3. In your JSON object, content property is an object itself. Why? An object of what? Which properties? It is printed in the template, but a mapping is not printable in Twig (except when it is a Drupal renderable, but those are expected in slots). Would a simple
string
fit instead?4. If you make
content
property a string, it would be the opportunity to adopt an existing Drupal data structure, like the menu structure, so it will be easier to plug it to Drupal API, by replacingcontent
bytitle
. - Status changed to Needs work
2 months ago 8:42pm 5 September 2024 - Status changed to Needs review
2 months ago 4:57pm 12 September 2024 - Status changed to Needs work
about 2 months ago 4:04pm 15 September 2024 - ๐ซ๐ทFrance pdureau Paris
Looks great.
label
propI would recommend to make it a slot instead of a prop to be able to send any renderable into it. But I know slots are a bit underappreciated in SDC those days, and this is not a big deal anyway.
tags
proptags: title: Tags type: array items: type: object properties: content: title: Tag type: string
Only the first one is a blocker in my opinion.
1. โ ๏ธ Now, we have an array of objects, where the object has only one property:
content
. Where didattributes
go? Forgotten I guess, because visible in the template:<li{{ tag.attributes.addClass('tags__item') }}>{{ tag.content }}</li>
2. As said previously, even without โจ Add unified Links data type for menus, pagers, breadcrumbs... Active , you can make integration with Drupal API (menus, links, breadcrumbs..) easier by using
text
ortitle
instead ofcontent
. - ๐ท๐ธSerbia finnsky
Getting tonns of errors like
[Tue Oct 22 20:50:22 2024] Uncaught PHP Exception Drupal\Core\Render\Component\Exception\InvalidComponentException: "[props.properties.tags.items.properties.attributes.type] Does not have a value in the enumeration ["array","boolean","integer","null","number","object","string"]/n[props.properties.tags.items.properties.attributes.type] String value found, but an array is required/n[props.properties.tags.items.properties.attributes.type] Failed to match at least one schema/n[props.properties.tags.items] Object value found, but an array is required/n[props.properties.tags.items] Failed to match at least one schema" at /Users/ivan/Sites/experiments/navi-core/core/lib/Drupal/Core/Theme/Component/ComponentValidator.php line 121
[Tue Oct 22 20:50:22 2024] [::1]:49327 Closingwhen defining attributes in component. via object or that weird Drupal related type