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

Home activity overview container #15

Merged
merged 9 commits into from
Jun 2, 2021

Conversation

paulotruta
Copy link
Member

@paulotruta paulotruta commented Jun 1, 2021

Fixing some details left after the symfony port merge for the home/index.twig.html activity overview widget:

  • Handle case where there is no activity on the database yet. This will happen when the install is fresh and edgeboxctl didn't yet filled activity information in the form of Tasks. It should display a small intro message explaining what that part of the interface will contain.
  • Port the widget to its own template file (that can be then easily included for easy code reutilization).

Looking at the twig documentation, two ways of doing this:

Both options seem to have advantages and disadvantages. Personally I think including a template (and using pass variables to the included template) seem the simplest, most straightforward option. This will be my first attempt.

@paulotruta paulotruta added the enhancement New feature or request label Jun 1, 2021
@paulotruta paulotruta requested a review from inverse June 1, 2021 23:06
@paulotruta paulotruta self-assigned this Jun 1, 2021
@paulotruta
Copy link
Member Author

paulotruta commented Jun 2, 2021

After creating a partial for the activity overview widget, got really excited about what we can reuse here!
Created an issue (#16) so we can follow up on creating partials from other elements of the interface that will or can possibly be re-used in other contexts.

@@ -179,7 +179,7 @@ private function getActionsOverviewContainerVars(): array {
$icon_color_class = 'danger';
break;
default:
$icon_color_class = 'default';
$icon_color_class = 'warning';
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this correct? Should we have another state for STATUS_CREATED?

Copy link
Member Author

Choose a reason for hiding this comment

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

This is a css color class to be applied in the template.
After testing, I noticed that the "default" class paints the icon color white, and it stays invisible.

Then I tested with "dark", which made the icon dark grey. This looked better (you could see the icon now), but the color created a mood in me which looked like the task was not executing properly or was stopped...

The "warning" class paints the icon in a shade of orange / yellow. Together with the message, it looks and feels better to have a colored icon indicating that the task is in being worked on.

Resuming, for this widget my observation of the color icons being used in conjunction with the message indicated:

Dark grey icon: Feeling of something static
Yellow icon: Feeliing of something being worked on
Green Icon: Feeling of success
Red Icon: Feeling of problem

Copy link
Member Author

Choose a reason for hiding this comment

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

And in the end, the created falls back to the default option of the switch statement, so it gets a yellow color.

We can introduce a case option for the created status as well, just to be more explicit (and maybe add a comment about the mood like I wrote above) :)

Copy link
Member Author

Choose a reason for hiding this comment

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

Added b725da3 for more clear handling of all cases.

Copy link
Contributor

@inverse inverse left a comment

Choose a reason for hiding this comment

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

One minor comment

@paulotruta paulotruta merged commit 3296768 into main Jun 2, 2021
@paulotruta paulotruta deleted the home-activity-overview-container branch June 2, 2021 11:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants