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

Decorator improvements #2654

Merged
merged 14 commits into from Aug 10, 2016

Conversation

Projects
None yet
2 participants
@edmundoa
Member

edmundoa commented Aug 10, 2016

This PR adds some improvements to decorators. I still have some open items in my list, but I think splitting them up will make the review process less annoying.

These are some of the changes included:

  • Improve overall appearance, hiding some elements in the decorator sidebar to gain some space, changing other UI elements to make them more obvious to users, and doing some margins and paddings work
  • Enforce user to select a pipeline to create a decorator from a pipeline
  • Only show "show original" button when message is decorated
  • Search response includes a list of all fields added, modified or removed
  • Hide field analysers and field value actions for all decorated fields, not only added field

@edmundoa edmundoa added this to the 2.1.0 milestone Aug 10, 2016

edmundoa added some commits Aug 8, 2016

Move decorators help to popover
In that way we gain some additional spacing on the sidebar.
Improve add decorator styling
Clean-up divs and improve margins
Move edit and delete buttons into actions dropdown
The decorator list doesn't have much space, and this makes it look
cleaner.
Do not add pipeline decorator without a pipeline
Mark non-optional select values as required, avoiding submitting a form
to create a pipeline decorator without selecting a pipeline.
Relax prop type in sortable list
Message decorators pass react elements instead of text, but that should
be allowed.
Do not display message field actions when decorated
Before this change, we didn't display message field actions for fields
added by a message decorator, but we did when a field was altered by a
decorator. This changes that behaviour, as decorated values will not
work on searches, and cannot be extracted.
Add modified fields to search decoration stats
We should also include changed and modified fields. And also show the
actual added fields.
Do not show field analyzers for decorated fields
Fields added by decorators did not show field analyzers, but modified
fields did. This removes that, as it won't work.
Use a more explicit indicator for decorated fields
Replace the pencil icon with some text that self explains why that field
is special.

@edmundoa edmundoa force-pushed the decorators-improvements branch from 1f99ba7 to d843a43 Aug 10, 2016

@kroepke kroepke self-assigned this Aug 10, 2016

@@ -40,14 +32,15 @@ const MessageField = React.createClass({
return (
<span>
<dt key={`${key}Title`}>{key} {this._decorationMarker(key)}</dt>

This comment has been minimized.

@kroepke

kroepke Aug 10, 2016

Member

minor nitpick, I'd leave the decorated marker here instead of underneath the value.
with the new marker it's actually quite readable (I guess this change was made before the icon was changed into a text)

screenshot

This comment has been minimized.

@edmundoa

edmundoa Aug 10, 2016

Member

The creative director (aka @mariussturm) and I were looking into some alternatives and the current approach is the one we liked the most: The idea was to be non-obtrusive when normally looking around message fields, as mostly you don't care if fields are decorated or not, while still having the information somewhere if you really needed to know. Having the (decorated) marker right next to the field may be too distracting, specially when several fields are modified by decorators.

Field terms: &nbsp;{this._getFormattedTerms()}
</Alert>
}
}
{this.props.isDecorated && <DecoratedMessageFieldMarker />}

This comment has been minimized.

@kroepke

kroepke Aug 10, 2016

Member

if we move the marker next to the field name, this should be removed

@kroepke

This comment has been minimized.

Member

kroepke commented Aug 10, 2016

other than the marker placement this lgtm

@edmundoa edmundoa changed the title from Decorators improvements to Decorator improvements Aug 10, 2016

@kroepke

This comment has been minimized.

Member

kroepke commented Aug 10, 2016

We agreed that we cannot agree on the placement comment above. So we choose to not agree and leave it like this.

good work!

@kroepke kroepke merged commit 48f96b9 into master Aug 10, 2016

4 checks passed

ci-server-integration Jenkins build graylog2-server-integration-pr 1244 has succeeded
Details
ci-web-linter Jenkins build graylog-pr-linter-check 727 has succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@kroepke kroepke deleted the decorators-improvements branch Aug 10, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment