New carousel component #179
Conversation
| 'Name of the carousel.', | ||
| 'TEXT', | ||
| TRUE, | ||
| FALSE | ||
| ), | ||
| ( | ||
| 'carousel', | ||
| 'title', | ||
| 'Title of the carousel.', |
There was a problem hiding this comment.
The docs does not make it clear what the difference between name and title is.
There was a problem hiding this comment.
I changed with 'An unique string to identify the caroussel component in the HTML page.' and 'A name to display at the top of the carousel.'
| ( | ||
| 'carousel', | ||
| 'indicators', | ||
| 'Style of image indicators (e.g. square, dot).', |
There was a problem hiding this comment.
We should list possible values exhaustively here.
There was a problem hiding this comment.
For the moment, the carousel supports only dots and square indicators. I changed with 'Style of image indicators (square or dot).'
examples/official-site/sqlpage/migrations/33_carousel_component.sql
Outdated
Show resolved
Hide resolved
| {{#if title}} | ||
| <div class="carousel-caption d-none d-md-block"> | ||
| <h5>{{title}}</h5> | ||
| {{#if contents}}<p>{{contents}}</p>{{/if}} |
There was a problem hiding this comment.
Can we add the possibility to render markdown descriptions ?
Like other components, with description and description_md
… is a short paragraph formatted using markdown.
|
Requested changes done |
| 'An unique string to identify the caroussel component in the HTML page.', | ||
| 'TEXT', | ||
| TRUE, | ||
| FALSE |
There was a problem hiding this comment.
We try to avoid having such requirements in sqlpage. We can do with it if there is no other choice, but it looks like this is an internal implementation technicality. Wouldn't it be better if the user didn't have to specify an id and make sure it is unique ? It is very easy to end up with duplicate identifiers and errors that are completely impossible to debug if you don't already understand html, javascript and the DOM. And the entire idea of sqlpage is to get rid of such prerequisites.
Couldn't we auto-generate an id like carousel_{{query_number}} where query_number comes from https://github.com/lovasoa/SQLpage/blob/main/src/render.rs ?
There was a problem hiding this comment.
Yes, it's possible to use an id like carousel_{{query_number}} but it seems to me not accessible in the template. If I understand the Rust code, the value of query_number is only initialized where an error is occured and only in development mode.
There was a problem hiding this comment.
Yes render.rs needs to be updated to add query_number to the handlebars context.
There was a problem hiding this comment.
You can look at how row_index is added to the context.
query_number needs to be passed all the way down to the SplitTemplateRenderer
lovasoa
force-pushed
the
main
branch
2 times, most recently
from
b1c6716 to
d38ca15
Compare
|
I reopened this is #214 and implemented the changes we talked about. |
* Initial carousel component implementation * change carousel example * make the carousel text description more readable by adding a background * update cat example * carousel automatic cycling is now optional * remove requirement for unique name attribute in carousel see #179 (comment) * fix carousel buttons * uodate deps * fix official site docs * lint * update changelog --------- Co-authored-by: Olivier Auverlot <@olivierauverlot>
No description provided.