General Style/Layout enhancements by flourish86 · Pull Request #103 · Icinga/icinga-notifications-web

flourish86 · 2023-05-22T11:59:52Z

Fixes some minor style and ui issues:

Clarifies button labels
Makes disabled state of buttons more obvious
Makes button styles more consistent

nilmerg

nilmerg · 2023-05-23T06:30:28Z

application/controllers/ContactsController.php

-                t('Add Contact'),
-                'notifications/contacts/add',
+                t('New Contact'),
+                'noma/contacts/add',


wrong module name

nilmerg · 2023-05-23T06:32:24Z

application/controllers/EventRulesController.php


    public function indexAction(): void
    {
+        $this->content->getAttributes()->add(['class' => 'full-width']);


Please move this to the bottom where the list is added to the content.

✅
Also did this for the other occurrences:

IncidentsController

EventsController

nilmerg · 2023-05-23T06:32:55Z

application/controllers/EventRulesController.php

-        $this->addTitleTab(t('Add Event Rule'));
-        $this->getTabs()->setRefreshUrl(Url::fromPath('notifications/event-rules/add'));
+        $this->addTitleTab(t('New Event Rule'));
+        $this->getTabs()->setRefreshUrl(Url::fromPath('noma/event-rules/add'));


wrong module name

nilmerg · 2023-05-23T06:34:03Z

application/controllers/EventRulesController.php

                'notifications/event-rules/add',
                'plus'
            ))->setBaseTarget('_next')
-            ->addAttributes(['class' => 'new-event-rule'])


Please also remove the class then

nilmerg · 2023-05-23T06:34:40Z

application/controllers/EventsController.php

    public function indexAction(): void
    {
        $this->addTitleTab(t('Events'));
+        $this->content->getAttributes()->add(['class' => 'full-width']);


to the bottom where the list is added please

nilmerg · 2023-05-23T06:35:08Z

application/controllers/IncidentsController.php

    public function indexAction(): void
    {
        $this->addTitleTab(t('Incidents'));
+        $this->content->getAttributes()->add(['class' => 'full-width']);


to the bottom where... please

nilmerg · 2023-05-23T06:46:15Z

library/Notifications/Widget/Schedule.php

                    [
                        new Icon('plus'),
-                        t('Add new entry')
+                        t('Add Entry')


You've standardized it everywhere else to New ..., why not here??

So the idea behind this is:

You create/delete an Object (e.g. "New Schedule”), when you’re in the “schedules” views (Create)

You add/remove “entry" objects to that “schedule", when you’re in the “schedules” view (Create and link)

This helps

to clarify the different relations and

with distinguishing the different actions better, especially in a view with many buttons in the same design

I decided against additionally highlighting the “add entry” button as a primary button, because it’s not that important in that view and would distract from the actual content which is the schedule itself.

Ah, okay, but I also add a new rule to the list of already existing event rules. I also add a new contact to the list of available contacts. In both cases, the relation is the list on the left. And to a list you add new items. The perfect label would be Add New Contact, but that's a bit redundant IMHO.

So if you think Add Entry is fine on the schedules view, I don't understand why you think Add Contact is not, on the contacts list.

Yes, don't you see the difference?

contact (list): new contact
schedule (list): new schedule
schedule: add entry

Not sure, how to make it clearer.

It also helps clarify the difference when there's that many different buttons of the same style, as shown on the screenshot.

I agree on the redundancy, especially as it doesn't add any value and distracts from the actual important term "Contact".

I don't want to discuss this to death. If you insist or it doesn't make sense to you, we can label it "+ New Entry".

nilmerg · 2023-05-23T07:07:54Z

public/css/form.less

Everything you've added in here should be moved to Icinga Web 2. Add it there to forms.less please.

While you do that, please make btn-default a standalone class (should not require btn-cancel). This should also solve the issue that once again there are no focus styles!

✅ As discussed “offline”, I dropped the custom styles for .form-controls.

nilmerg · 2023-05-23T07:14:18Z

public/css/list/item-list.less

This is a well tested and proven layout. Your changes break overflow and lead to different sized gaps:

Please revert all changes in this file.

Sadly, you’re right. But inconsistent spacing was the reason I went for this in the first place.

flourish86 requested a review from nilmerg May 22, 2023 11:59

cla-bot bot added the cla/signed CLA is signed by all contributors of a PR label May 22, 2023

flourish86 force-pushed the fix/address-style-issues branch from c3c3c8a to 28475d4 Compare May 22, 2023 12:24

nilmerg requested changes May 23, 2023

View reviewed changes

nilmerg assigned flourish86 May 23, 2023

nilmerg added enhancement New feature or improvement area/ui Affects the user interface labels May 23, 2023

nilmerg changed the title ~~Fix/address style issues~~ General Style/Layout enhancements May 23, 2023

flourish86 added 14 commits May 23, 2023 11:27

CSS: Align .cache-notice

4bfa529

EventListItem: Do not render footer

46a51db

CSS: Align EventListItem in controls vertically

da0cbf5

list-item.less: Do not wrap .meta element

f2fc031

event-source-badge: Optimize colors in light mode

c02bed6

CSS: Optimize .entry .title spacing

040fd79

Add .full-width class to .content containers

e9ea20a

Label create buttons consistently

c8d21b5

Clarify button labels in forms

308fda7

Realign buttons in .form-controls

b96e2bc

CSS: Clarify disabled state

eb65ebd

ChannelForm: Clarify delete button label

2852a4d

ContactForm: Clarify delete button label

e45eb70

event-rule-details.less: Remove unused `. new-event-rule

4b33a04

flourish86 force-pushed the fix/address-style-issues branch from 69643e2 to 4b33a04 Compare May 24, 2023 09:15

Make channel form button labels consistent

d06d36c

flourish86 requested a review from nilmerg May 24, 2023 13:49

Conversation

flourish86 commented May 22, 2023

Uh oh!

nilmerg left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

flourish86 May 23, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

flourish86 May 23, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

flourish86 May 23, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

flourish86 May 23, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

flourish86 May 23, 2023 •

edited

Loading

flourish86 May 23, 2023 •

edited

Loading

flourish86 May 23, 2023 •

edited

Loading

flourish86 May 23, 2023 •

edited

Loading