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

[UX] Revamp the Syslog settings form #4615

Open
klonos opened this issue Sep 12, 2020 · 9 comments · May be fixed by backdrop/backdrop#3303
Open

[UX] Revamp the Syslog settings form #4615

klonos opened this issue Sep 12, 2020 · 9 comments · May be fixed by backdrop/backdrop#3303

Comments

@klonos
Copy link
Member

klonos commented Sep 12, 2020

Enable the syslog module -> navigate to /admin/config/development/logging

That list makes the form unnecessarily long. I propose that for consistency, we re-use the same UI pattern as the one used for the "Rewrite the output of this field" option in the Views UI:

@klonos klonos self-assigned this Sep 12, 2020
@klonos klonos added the design label Sep 12, 2020
@klonos
Copy link
Member Author

klonos commented Sep 12, 2020

Don't you just love it when you start wanting to tweak something seemingly simple, and then you get into one rabbit hole after another? 😅 ...

So here's where I want to get this at ideally:

  • Remove the "Specify the format of the syslog entry" help text, as it is providing no value. Rename the label of the text area from "Syslog format" to "Syslog entry format" instead.
  • The fieldset is collapsed by default, to save on vertical space and unnecessary scrolling (the original goal of this issue).
  • There is no border, nor any white background in the fieldset, to match how the "Replacement patterns" fieldset is styled in the Views UI (consistency please 🙂). Besides, having the usual border and background would make this seem like a stand-alone setting, separate from the text area (more on that later).

...when you expand the fieldset, you get this:

  • The variables are rendered as a list (readability)
  • The actual variables are bolded and wrapped in <code> tags (which renders them in a different font), to tell them apart from their descriptions (readability/scannability).
  • I have retained the = as the separator between the variable and its respective description, as adding : instead could result in people thinking that the colon is part of the variable. I think that the intention was the same with using = as the separator between the tokens and their descriptions in the Views UI. I need more feedback on that, but whatever we decide, we should implement the same in both places (and in any other place we use same patterns) for consistency.

@klonos
Copy link
Member Author

klonos commented Sep 12, 2020

Now, for the rabbit holes I've mentioned in my previous comment...

  1. The style that is responsible for rendering the "Replacement patterns" fieldset in the Views UI without any border or background is in https://github.com/backdrop/backdrop/blob/1.x/core/themes/seven/css/views-admin.seven.css#L25

    fieldset fieldset {
      border: medium none;
    }

    I want to reuse this here, so should we move that to https://github.com/backdrop/backdrop/blob/1.x/core/themes/seven/css/style.css instead, so that there's no duplication?

    PS, when opening a Views UI dialog in a separate browser tab (to simulate no-js), this no-border style is not applied:

    Screen Shot 2020-09-12 at 8 02 13 am

    ...should we fix all that in one go?

@klonos
Copy link
Member Author

klonos commented Sep 12, 2020

...I can see someone chiming in to say "just leave things as they are, with the default fieldset style". If we do that, then we'd end up with this:

...which as I mentioned earlier, makes it look as though that fieldset is holding some stand-alone setting. So I've tried to put the text area and the "Replacement variables" in a non-collapsible fieldset, to group them together. The result is this:

This conveys the grouping much clearer, but the "Syslog entries" label of the parent fieldset provides no value, and eats up vertical space on the form, which defeats the purpose of this issue 😓 ...so I have tried to hide it with '#title_display' => 'invisible' ...but that doesn't work, because #4616 😓 😓 😓 😓

So should we group things, and hide the fieldset label via CSS, then add a @to-do to revisit when we fix #4616?

@klonos
Copy link
Member Author

klonos commented Sep 12, 2020

...ooh ooh ooh, did I mention that the descriptions of the syslog variables were not translatable before, and now they are? 😉

PS: also removed some extraneous spaces between key/values in arrays in syslog.module, that were breaking our coding standards + removed reference to help text being added via the Help module (which we have removed in Backdrop).

@klonos
Copy link
Member Author

klonos commented Sep 12, 2020

PR up for review/feedback: backdrop/backdrop#3303

@klonos
Copy link
Member Author

klonos commented Sep 12, 2020

...I will also see if I can make it so that these variables can be added via a modal, like the one we use for token replacement. In that case, the whole fieldset can go away, and we can add a "variable browser" link in the help text of the text area.

@ghost
Copy link

ghost commented Jun 3, 2022

There's at least one typo and conflict that needs resolving.

@klonos klonos changed the title [UX] Better variables legend for the syslog format [UX] Revamp the Syslog settings form Dec 27, 2022
@klonos
Copy link
Member Author

klonos commented Dec 27, 2022

Thanks for reviewing @BWPanda 🙏🏼 ...sorry for taking long to reply, but I've updated the PR now. Here's where it's at currently:

image

Summary of changes:

  • Wrapped all syslog-related fields in the form with a fieldset (we are doing the same for dblog-related fields in Add a configuration option for database log message display length. #5553, so things will be consistent)
  • Removed the $help bit that was previously meant to be added to the description text of the "Syslog identity" and the "Syslog facility" fields, as it was dependant on the Help module (which has been removed from Backdrop).
  • Replaced "A string that will be prepended to every message..." in the help text with "A prefix added to every message..." ("string" is a nerdy/developer term that should be not be used in UI text when possible).
  • Removed spacing that was added before => in arrays for "pretty formatting", as this is against our coding standards.
  • Converted the legend with variables to a "pseudo-token" browser! This is the big UX change here, as a) it reduces the length of the form by moving the contents of the legend into a token browser, taking them out of the way when not needed and b) allows people to click to add variables à la token-style, instead of having to copy/paste them.
  • Still keeps the log format field as a '#type' => 'textarea' (since syslog entries can be multi-line), but reduces it to be rendered in 1 line by default. You can still make the log entry multi-line as before, so there is no functionality loss, but you will need to manually drag the text area handle to increase it. This change helps reduce the length of the form a bit more.
  • I have added a description to suggest that log entries should be kept a single line where possible, as best practice. Besides, various sources around the net suggest that by default rsyslog removes new lines anyway, and it needs special tweaking to allow them - see https://stackoverflow.com/questions/5463992/multiline-log-records-in-syslog for example.
  • I've removed syslog_logging_settings_submit() as it was doing absolutely nothing useful. In fact, it was saving all settings in system.core.json as another duplicate set of syslog_* entries, while we have existing entries for them in the same config file as log_*. This is a pre-existing bug that I will file a follow-up issue for. ...no syslog_* config is being used anywhere in our codebase, and as also mentioned in Add a configuration option for database log message display length. #5553 we need a follow-up issue to move the dblog and syslog settings out of system.core.json and into their own respective syslog.settings.json and dblog.settings.json files, so that these settings can be removed when these modules are uninstalled.

This is ready for review/testng/feedback.

@jenlampton
Copy link
Member

Just gonna link this here: #1260

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

Successfully merging a pull request may close this issue.

2 participants