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.

Sign up for GitHub

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

Jump to bottom

EZP-28386: Update Create & Edit design layout #388

Merged
merged 1 commit into from
Mar 21, 2018
Merged

EZP-28386: Update Create & Edit design layout #388

merged 1 commit into from
Mar 21, 2018

Conversation

inakijv
Copy link
Contributor

@inakijv inakijv commented Mar 13, 2018

Question Answer
Tickets EZP-28386
Bug fix? no
New feature? no
BC breaks? no
Tests pass? no
Doc needed? no
License GPL-2.0

Most important aspects:

  • Updated Create and Design layouts based on UI Guidelines design;
  • Close button placed on left side, in order to avoid usability issues due to the closeness of Close and Discard buttons when the first one is on the right side;
  • Form fields contained within ez-card following consistency with Editing view in Admin section.
  • Added offset margins to Creating/Editing language name, Content item title, Content Type name and Content Type descriptions;
  • Updated color for Creating/Editing language name according to color contrast standards defined in UI Guidelines;
  • Added offset margins to content_edit_component_preview_unavailable.

ez platform 31
ez platform 30
ez platform 29
ez platform 28
ez platform 27

Next steps:

  • Flex Workflow notifications in EZEE should be adapted to this new layout too.

Checklist:

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

SylvainGuittard
SylvainGuittard reviewed Mar 13, 2018
View reviewed changes
Copy link
Contributor

@SylvainGuittard SylvainGuittard left a comment

Choose a reason for hiding this comment

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

Looks better. Thanks @inakijv

src/bundle/Resources/public/scss/_content-edit.scss Outdated
h2,
h4 {
font-size: 1.125rem;
color: $ez-color-base-light;
Copy link
Contributor

Choose a reason for hiding this comment

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

please remove the extra space :)

sunpietro
sunpietro suggested changes Mar 14, 2018
View reviewed changes
src/bundle/Resources/public/scss/_content-edit.scss Outdated
@@ -1,10 +1,18 @@
.ez-content-edit {
h2,
Copy link
Contributor

Choose a reason for hiding this comment

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

Please, avoid styling by tag selectors. Use CSS class selectors instead. In case of there's none, please add a new one.

@alongosz alongosz mentioned this pull request Mar 14, 2018
Closed
2 tasks
@inakijv
Copy link
Contributor Author

inakijv commented Mar 14, 2018

Update based on review. Ping @sunpietro

@lserwatka
Copy link
Member

@inakijv rebase is needed here.

@inakijv
Copy link
Contributor Author

inakijv commented Mar 16, 2018

I was updated based on feedback. Still pending new review.

@inakijv
Copy link
Contributor Author

inakijv commented Mar 19, 2018

Ping @sunpietro

@sunpietro sunpietro requested review from mnocon and dew326 March 19, 2018 13:33
@sunpietro sunpietro assigned sunpietro and inakijv and unassigned sunpietro Mar 19, 2018
dew326
dew326 requested changes Mar 19, 2018
View reviewed changes
src/bundle/Resources/public/scss/_content-edit.scss Outdated
@@ -1,14 +1,21 @@
.content-item-status {
Copy link
Member

Choose a reason for hiding this comment

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

Wrong naming convention ez-content-item-status

@inakijv
Copy link
Contributor Author

inakijv commented Mar 19, 2018

Thanks for the feedback 👍 Updated based on review

@lserwatka
Copy link
Member

@mnocon we need a round of QA on this one too.

@m-tyrala m-tyrala self-assigned this Mar 20, 2018
m-tyrala
m-tyrala suggested changes Mar 20, 2018
View reviewed changes
Copy link
Contributor

@m-tyrala m-tyrala left a comment •
edited
Loading

Choose a reason for hiding this comment

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

There's a problem with styling in map field, with address field and loupe icon.
screen shot 2018-03-20 at 20 32 47

@inakijv
Copy link
Contributor Author

inakijv commented Mar 20, 2018

Proposed fixup based on @m-tyrala feedback (good call btw!). Tested on latest versions of Chrome and Firefox.

screen shot 2018-03-20 at 4 55 52 pm

@SylvainGuittard
Copy link
Contributor

For me, the button is a little bit bigger than the input box. Is it something we can improve?

@inakijv
Copy link
Contributor Author

inakijv commented Mar 20, 2018

Updated based on @SylvainGuittard feedback

screen shot 2018-03-20 at 5 23 02 pm

@lserwatka
Copy link
Member

@m-tyrala it is ready for re-test.

@m-tyrala
Copy link
Contributor

I have noticed also, that there's problem in this PR with checkbox labels in Content Type creation - we see small label shift but only before you add first field (after that all is fine, even if we delete this one added field). Maybe it's just the matter of rebase? We was dealing with problems alike last days.
screen shot 2018-03-21 at 09 27 52

@mnocon
Copy link
Member

mnocon commented Mar 21, 2018

It might be an independent issue (see: https://jira.ez.no/browse/EZP-28974)
ping @barbaragr - I guess this is also present in RC1?

@lserwatka
Copy link
Member

I will merge this one, and checkbox issue will be addressed by one person.

@sunpietro
Copy link
Contributor

MAybe this PR is not rebased with the latest master. It was fixed in #400 as far as I know.

@lserwatka
Copy link
Member

@sunpietro do you have possibility to rebase this PR?

m-tyrala
m-tyrala approved these changes Mar 21, 2018
View reviewed changes
Copy link
Contributor

@m-tyrala m-tyrala left a comment

Choose a reason for hiding this comment

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

So just as I thought - the rest seems to be fine - QA approve.

@mnocon
Copy link
Member

mnocon commented Mar 21, 2018 •
edited
Loading

#400 was merged 2 days ago, the issue I linked (created by @barbaragr ) was added yesterday
with Slack comment:
(znowu skądś wylazło, ostatnim razem było w porządku) 😛
I don't think rebase will help here

@sunpietro
Copy link
Contributor

I've rebased this branch. Please, check it again.

@lserwatka lserwatka merged commit 888790c into ezsystems:master Mar 21, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@SylvainGuittard SylvainGuittard SylvainGuittard approved these changes

@sunpietro sunpietro sunpietro approved these changes

@dew326 dew326 dew326 approved these changes

@m-tyrala m-tyrala m-tyrala approved these changes

@mnocon mnocon Awaiting requested review from mnocon

Assignees

@inakijv inakijv

@m-tyrala m-tyrala

Labels
Improvement Ready for QA
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@inakijv @lserwatka @SylvainGuittard @m-tyrala @mnocon @sunpietro @dew326