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

Front 2490: Table pattern #78

Merged
merged 6 commits into from
Oct 1, 2021
Merged

Front 2490: Table pattern #78

merged 6 commits into from
Oct 1, 2021

Conversation

Maxfire
Copy link
Contributor

@Maxfire Maxfire commented Sep 24, 2021

@Maxfire Maxfire changed the title Front 2490 Front 2490: Table pattern Sep 24, 2021
@Maxfire Maxfire force-pushed the FRONT-2490 branch 2 times, most recently from 20568b0 to 285f385 Compare September 24, 2021 12:27
Copy link
Contributor

@abel-santos-corral abel-santos-corral left a comment

Choose a reason for hiding this comment

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

Review finished. Only added +1 row at table.ui_patterns.yml and changed the content of the three rows of the preview.

{% set rows_items = rows_items|merge([{'cells': row_content}]) %}
{% endfor %}
{{ pattern('table', {
caption: caption,
Copy link
Contributor

Choose a reason for hiding this comment

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

Missing table_responsive:

Suggested change
caption: caption,
caption: caption,
table_responsive: responsive ? true...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As we checked it's already in place.

caption: caption,
rows: rows_items,
table_head: table_head,
}) }}
Copy link
Contributor

Choose a reason for hiding this comment

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

Details (summary and description) also missing inside caption.

Copy link
Contributor

@escuriola escuriola Sep 29, 2021

Choose a reason for hiding this comment

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

Checking with "seven" theme, seems to be a collapsible field. The summary is the link and the description the content.
image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added in the view template.

</table>
</div>
{{ pattern('table', {
caption: caption,
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
caption: caption,
caption: caption,
table_responsive: true,

Copy link
Contributor Author

Choose a reason for hiding this comment

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

same as before, it's already in place.

@Maxfire Maxfire force-pushed the FRONT-2490 branch 2 times, most recently from 791ae10 to 36604c9 Compare September 30, 2021 09:33
@drishu drishu merged commit 6f5f60b into 1.x Oct 1, 2021
@drishu drishu deleted the FRONT-2490 branch October 1, 2021 12:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants