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

ENHANCEMENT Show state of elements draft/live/modified #374

Conversation

raissanorth
Copy link
Contributor

Styling based on current implementation:

image

Note that the CSS changes minimally increase the distance between the header title and the summary content.

@raissanorth
Copy link
Contributor Author

Not sure what is causing the Behat test failures. Could be a timing issue. That particular test runs fine locally:

image

@raissanorth raissanorth force-pushed the pulls/master/versioned-state-display branch from bd486fa to 65bfa37 Compare September 5, 2018 04:02
@raissanorth
Copy link
Contributor Author

Added a box-shadow to the circle icon. Unfortunately it is a lot more subtle than the one in the designs, since I tried to reuse existing variables for consistency:

image

Copy link
Contributor

@robbieaverill robbieaverill left a comment

Choose a reason for hiding this comment

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

I've made some suggestions. I'll add a commit to this PR with my suggested changes, feel free to blow them away if you don't like them though =)

'element-editor-header__version-state',
{
'element-item--draft': !isPublished,
'element-item--modified': isPublished && !isLiveVersion,
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should make the state a modifier on whatever the element already is. I think it'd be useful to apply this from the element-editor__element element e.g. element-editor__element--modified or element-editor__element--published then propagate it down.

The CSS could then look like this:

.element-editor__element {
  &--modified {
    .element-editor-header__version-state {
      // apply styles here for the circle
    }
  }
}

}

if (isPublished && !isLiveVersion) {
versionStateButtonTitle = 'Item has unpublished changes';
Copy link
Contributor

Choose a reason for hiding this comment

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

These need to be translatable, and I think they could also be in their own method

@@ -32,5 +32,7 @@ Feature: Edit elements in the CMS
And I press the "Publish" button
Then I should see a "Published content block" message
When I go to "/admin/pages/edit/show/6"
Then I should see "Eve's Block"
And I wait until I see the ".element-editor__element" element
Copy link
Contributor

Choose a reason for hiding this comment

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

I think I mentioned this on another PR - this assertion isn't designed to select a class - you're basically saying "wait until I see the [any element] element" where it's designed to be more specific than that e.g. "wait until I see the [specific] element".

We could add another step like "wait until the elements have loaded" instead maybe?

@robbieaverill
Copy link
Contributor

By the way, I realise re: the class names that you're re-using existing styles from the PHP GridField equivalent, but I think we should migrate them to the React structure and update the legacy reports GridField implementation to use those instead

@robbieaverill robbieaverill self-assigned this Sep 5, 2018
…s, make Behat look wait for elements to be rendered
Copy link
Contributor

@robbieaverill robbieaverill left a comment

Choose a reason for hiding this comment

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

@raissanorth if you're happy with my new commit then merge at your leisure

@robbieaverill robbieaverill removed their assignment Sep 5, 2018
@raissanorth
Copy link
Contributor Author

Very happy indeed, so will leisurely merge ;)

@raissanorth raissanorth merged commit 9462cb3 into silverstripe:master Sep 5, 2018
@raissanorth raissanorth deleted the pulls/master/versioned-state-display branch September 5, 2018 21:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants