-
Notifications
You must be signed in to change notification settings - Fork 1
Feature/info block #73
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
Conversation
rimi-itk
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This works and looks good, but I have some suggestions on how
- I think we should have a header on an info block
- I think we should use buttons (rather than list) to add a new paragraph
- I think “Attributes” should be replaced with a dropdown list with semantic categories, e.g. “default” and “highlight”. Contentwise “Bordered” doesn't really mean anything, whereas “highlight” (and “Did you know?” and the like) have a semantic meaning that we can use to style the content, e.g. with a border if the designer chooses to do so.
…feature/info-block
| bundle: os2loop_section_page_info_block | ||
| label: 'Information block' | ||
| description: '' | ||
| required: false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should the text be required? I vote “Yes!”
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not just let the user decide if they want to add text?
They may want to just add a section with a h2 . There will often be a h1 right above it.
I vote "no!"
| bundle: os2loop_section_page_info_block | ||
| label: Title | ||
| description: '' | ||
| required: false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should the title be required? I vote “Yes!”
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not just let the user decide if they want to add a h2 or not above the text?
Not all content needs a header. There will often be a h1 right above it.
I vote "no!"
| field_name: os2loop_section_page_attribute | ||
| entity_type: paragraph | ||
| bundle: os2loop_section_page_info_block | ||
| label: Attributes |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should rename the field to os2loop_section_page_attribute – or at least change the label to “Type” (which I think is a better name for what it is).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed label
| <div{{ item.attributes }}>{{ item.content }}</div> | ||
| {% endfor %} | ||
| </div> | ||
| {% for item in items %} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've wanted to make this change, but I didn't have the guts to do it – or the overview of the templates.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I too am unsure if it will break something, but i think it needs to be done to have proper html for a default field
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We will have to look out for weird label styling
https://jira.itkdev.dk/browse/LOOP-738
Add info block paragraph