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-29811: Content field definitions is misaligned while creating Content Type #790

Conversation

tischsoic
Copy link
Contributor

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

Fixes button styling. After changes:

screen shot 2019-01-07 at 08 38 43

screen shot 2019-01-07 at 08 45 22

@tischsoic
Copy link
Contributor Author

Styled using flex - ping @sunpietro @dew326

height: 1.5rem;
margin-top: 5px;
}
.ez-icon-trash {
Copy link
Member

Choose a reason for hiding this comment

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

You can use class ez-icon--medium to set 24px height and width

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, but I have decided to add .ez-icon-trash instead, because in create.html.twig this statement:

{{ form_widget(form.removeFieldDefinition, {'attr': {'class': 'btn btn-danger'}}) }}

generates button with .ez-icon-trash. Also, other trash buttons in our system have .ez-icon-trash class. For consistency, I think we should use this class here as well.

Copy link
Member

Choose a reason for hiding this comment

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

But adding class to set width and height is redundant. For having icons with width and height we have special modifier class ez-icon--medium. I'm not against adding .ez-icon-trash, I'm against using this class to set 24px width and height.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're right, better idea would be to use .ez-icon--medium. Also, I realised that the template for {{ form_widget(form.removeFieldDefinition ... is in the same file, so I can add .ez-icon--medium there.
Unfortunately, after applying changes I realised that we are styling .ez-card__header .form-inline .btn-danger .ez-icon in _icons.scss:

.ez-card__header {
.form-inline {
.btn-danger {
.ez-icon {
width: calculateRem(20px);
height: calculateRem(20px);

what overwrites .ez-icon--medium selector. Probably this is why someone before me styled it here using this selector: .ez-card__header--sticky-top .form-inline .btn-danger .ez-icon (I only changed .ez-icon -> .ez-icon-trash and moved out from --sticky-top).

So, I think that my current solution is valid in context of this task. Definitely, it would be great to refactor .ez-card__header .form-inline .btn-danger .ez-icon from _icons.scss, but I think that it should be done in another task/PR.
What do you think, @dew326 ? 🙂

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe you should fix that selector as well and apply a proper CSS class the elements defined by .ez-card__header .form-inline .btn-danger .ez-icon selector?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I checked what elements are styled with .ez-card__header .form-inline .btn-danger .ez-icon from _icons.scss and it turned out that it affects only two buttons which I'm fixing in this PR. I was suprised a little bit. 😄

Long story short, I removed .ez-card__header .form-inline .btn-danger .ez-icon from _icons.scss and styled buttons using ez-icon--medium & ez-icon--light. 🙂 👍

@tischsoic
Copy link
Contributor Author

ping @sunpietro @dew326

@lserwatka
Copy link
Member

@dew326 could you make a final review here? So we can move this to QA?

@lserwatka
Copy link
Member

@tischsoic could you change target branch to 1.4?

@tischsoic tischsoic changed the base branch from master to 1.4 January 16, 2019 14:23
@tischsoic
Copy link
Contributor Author

Target branch changed to 1.4 - ping @lserwatka

@katarzynazawada
Copy link

@tischsoic according to Affects Version base here should be 1.3. Please, do the rebase also :)

@tischsoic tischsoic force-pushed the EZP-29811-Content-field-definitions-is-misaligned-while-creating-CT branch from ac88b4b to b846455 Compare January 23, 2019 09:26
@tischsoic tischsoic changed the base branch from 1.4 to 1.3 January 23, 2019 09:27
@tischsoic
Copy link
Contributor Author

Changed target branch to 1.3 - ping @katarzynazawada

@@ -6,7 +6,7 @@

{% block _ezrepoforms_contenttype_update_removeFieldDefinition_widget %}
<button type="{{ type|default('button') }}" {{ block('button_attributes') }}>
<svg class="ez-icon ez-icon-trash">
<svg class="ez-icon ez-icon--medium ez-icon--light ez-icon-trash">
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this class: ez-icon-trash used anywhere in CSS or JS?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually it is not used, I will remove it.

@lserwatka lserwatka deleted the EZP-29811-Content-field-definitions-is-misaligned-while-creating-CT branch January 23, 2019 13:47
@lserwatka
Copy link
Member

@tischsoic could you merge it up?

@lserwatka
Copy link
Member

1.4 -> master

@tischsoic
Copy link
Contributor Author

Merged up:

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.

5 participants