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

EZP-29117: Improve Content Type view design #448

Merged
merged 4 commits into from
Apr 20, 2018

Conversation

mikadamczyk
Copy link
Contributor

Question Answer
Tickets https://jira.ez.no/browse/EZP-29117
Bug fix? no
New feature? yes
BC breaks? no
Tests pass? yes
Doc needed? no
License GPL-2.0

screenshot-2018-4-19 ez platform

Checklist:

  • Coding standards ($ composer fix-cs)
  • Ready for Code Review

@SylvainGuittard
Copy link
Contributor

Amazing! Thanks @mikadamczyk for the improvement.

@@ -74,7 +74,7 @@
color: $ez-color-base-dark;
}

.ez-info-view, .ez-link-manager-view {
.ez-info-view, .ez-link-manager-view, .ez-content-type-view {
Copy link
Member

Choose a reason for hiding this comment

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

Please move it to a new line, it's more readable.

</tr>
</thead>

Copy link
Member

Choose a reason for hiding this comment

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

Not needed empty line


<br />
<br />
Copy link
Member

Choose a reason for hiding this comment

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

I know it's not yours but this br looks odd. Can you check if it's needed?

@mikadamczyk mikadamczyk force-pushed the EZP-29117 branch 2 times, most recently from ba8fc5f to 0145ae1 Compare April 19, 2018 13:09
@dew326 dew326 requested a review from sunpietro April 20, 2018 07:11
@@ -74,7 +74,9 @@
color: $ez-color-base-dark;
}

.ez-info-view, .ez-link-manager-view {
.ez-info-view,
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe, instead of adding parent selectors you should create an instance of modifier class? It becomes to coupled to a specific context that is against BEM rules.
A thing to consider @dew326

@@ -117,6 +119,14 @@
}
}

.ez-content-type-view {
.ez-table-field-definitions {
thead th {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you avoid using tag selectors? Please, create a CSS class attached to th elements and use it instead.

<br />
<div class="container mt-4 px-4">
<section class="ez-fieldgroup">
<h2 class="ez-fieldgroup-name">{{ "content_type.content_field_definitions"|trans|desc("Content field definitions") }}</h2>
Copy link
Contributor

Choose a reason for hiding this comment

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

ez-fieldgroup-name is against BEM rules. You have to improve it. I suggest converting it to: ez-fieldgroup__name

</div>
<section class="container mt-4 px-5">
{% for group, field_definitions in field_definitions_by_group %}
<header class="ez-table-header ez-table-header">
Copy link
Contributor

Choose a reason for hiding this comment

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

Duplicated classes

Copy link
Contributor

Choose a reason for hiding this comment

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

It should be ez-table__header


<table class="table ez-table-field-definitions">
Copy link
Contributor

Choose a reason for hiding this comment

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

ez-table__field-definitions

@mikadamczyk
Copy link
Contributor Author

ping @sunpietro

Copy link
Contributor

@sunpietro sunpietro left a comment

Choose a reason for hiding this comment

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

Please, review your Twig updates once again.


<table class="table">
<thead>
<table class="table ez-table--list table">
Copy link
Contributor

Choose a reason for hiding this comment

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

Duplicated CSS class

{{ macros.content_type_edit(content_type, content_type_group, 'btn btn-primary') }}
</div>
<header class="ez-table__header">
<div class="ez-table-header__headline">{{ "content_type.content_type"|trans|desc("Content Type") }}</div>
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of ez-table-header__headline set ez-table__headline

mikadamczyk and others added 3 commits April 20, 2018 11:44
EZP-29117: Improve content type view design - miko

EZP-29117: Improve content type view design -miko
width: calc(100% / 3);
}
}
}

.ez-table-header {
Copy link
Contributor

Choose a reason for hiding this comment

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

It can be removed now.

Copy link
Member

@micszo micszo left a comment

Choose a reason for hiding this comment

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

So far so good. ✨
Waiting for final commit.

Copy link
Contributor

@sunpietro sunpietro left a comment

Choose a reason for hiding this comment

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

Looks like there are naming inconsistencies in many other files, unrelated to this PR. For this PR +1

@micszo micszo self-assigned this Apr 20, 2018
@lserwatka
Copy link
Member

@micszo can we merge it?

@micszo micszo removed their assignment Apr 20, 2018
@lserwatka lserwatka merged commit 00afb09 into ezsystems:master Apr 20, 2018
@mnocon mnocon mentioned this pull request Apr 23, 2018
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

7 participants