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

[UI] Replace header icons from FontAwesome to Remix #13624

Merged
merged 15 commits into from
Apr 19, 2024

Conversation

andersonjeccel
Copy link
Contributor

@andersonjeccel andersonjeccel commented Apr 9, 2024

Q A
Bug fix? (use the a.b branch) [ ]
New feature/enhancement? (use the a.x branch) [ ]
Deprecations? [ ]
BC breaks? (use the c.x branch) [ ]
Automated tests included? [ ]
Related user documentation PR URL mautic/mautic-documentation#...
Related developer documentation PR URL mautic/developer-documentation#...
Issue(s) addressed Fixes #...

Description:

Replaced to the new icon pack.
Added the user's position above their name.


Reordered the notification icon to improve UX.

In the matter of information distribution, we easily realize that items on the right side of the screen are more related to technical management or administration. Either resources such as categories as well as the settings tab, having them all in one side tells users we have a consistency.

So from this, we can think it would also make sense to keep the notification panel icon on the right side of the screen, since often it is responsible for showing information as a new update available.

The left side, on the other hand, commonly presents items that marketers use in everyday work, like contacts, campaigns as well as other necessary resources for the success of a strategy. I believe that keeping only the search icon alone in this side improves consistency, since they will use global search to find these items easily.

Before:

image

image

image

After:

microanimation.mp4

image

image

image

Steps to test this PR:

  1. Open this PR on Gitpod or pull down for testing locally (see docs on testing PRs here)
  2. Look at the navbar

@andersonjeccel andersonjeccel self-assigned this Apr 9, 2024
@andersonjeccel andersonjeccel added T1 Low difficulty to fix (issue) or test (PR) user-interface Anything related to appearance, layout, and interactivity code-review-needed PR's that require a code review before merging user-experience Anything related to related to workflows, feedback, and navigation enhancement Any improvement to an existing feature or functionality labels Apr 9, 2024
Copy link
Contributor

@LordRembo LordRembo left a comment

Choose a reason for hiding this comment

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

I understand the purpose of these changes but my 1 suggestion would be to split this PR into 3 separate PR's. You are combining 3 changes that are not interdependent and some are a bigger change than others.

  • moving the notification icon will be a point of contention and should be discussed before making a (separate) PR for it.
  • showing the user position/function with the name is a nice improvement but has nothing to do with icons

@code5rick
Copy link
Contributor

Love the animation that you made @andersonjeccel to show the user position!

Copy link
Contributor

@shinde-rahul shinde-rahul left a comment

Choose a reason for hiding this comment

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

Looks good.

@andersonjeccel
Copy link
Contributor Author

@LordRembo I'm thinking of the previous conversation about finding a balance between the difference/change size and testing efforts…

What if try this approach this time to see if people feel happy? If it goes wrong, I cherry pick these commits to another PR and separate them

@escopecz
Copy link
Sponsor Member

Some conflicts here too

@escopecz escopecz added the has-conflicts Pull requests that cannot be merged until conflicts have been resolved label Apr 18, 2024
@andersonjeccel andersonjeccel removed the has-conflicts Pull requests that cannot be merged until conflicts have been resolved label Apr 18, 2024
@andersonjeccel
Copy link
Contributor Author

@escopecz Solved!

@andersonjeccel
Copy link
Contributor Author

@RCheesley Mapped these images on user docs:

translations-select-language
translations-select-user-language
mautic-report
need-to-enable-composer
marketplace-detail
dashboard-date-filters
gui-update-deprecated
notifications
asset_create
asset_settings
dwc_create
Mautic-31-company-view
social_monitoring_hashtags
sms_send_marketing_message
create_sms
social_monitoring_mentions
focus_item_create
new_segment_sms
new_template_sms

Notifications is the only/most relevant since it's about its location, is feasible to recapture others again now?

@andersonjeccel andersonjeccel added code-review-passed PRs which have passed code review and removed code-review-needed PR's that require a code review before merging labels Apr 18, 2024
Copy link
Sponsor Member

@RCheesley RCheesley left a comment

Choose a reason for hiding this comment

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

I love these changes. I think it makes total sense to move the notifications over to the right (I never knew why they were on the left) and the animation on the account is a nice addition.

One thing though, and I know I am being totally pedantic here, but I think it should maybe have a max width for the description after which maybe it should wrap - otherwise this happens: https://app.screencastify.com/v3/watch/CwBm7ATnYWvCc4jZypdl

That can be addressed in another PR though if folks think it needs fixing ;)

@RCheesley RCheesley added the pending-test-confirmation PR's that require one test before they can be merged label Apr 19, 2024
@RCheesley
Copy link
Sponsor Member

Notifications is the only/most relevant since it's about its location, is feasible to recapture others again now?

@andersonjeccel I am not sure I fully understand the question here?

Copy link

codecov bot commented Apr 19, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 61.28%. Comparing base (3051381) to head (48acca4).

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff              @@
##                5.x   #13624      +/-   ##
============================================
- Coverage     61.29%   61.28%   -0.01%     
  Complexity    33998    33998              
============================================
  Files          2238     2238              
  Lines        101631   101631              
============================================
- Hits          62290    62289       -1     
- Misses        39341    39342       +1     

see 1 file with indirect coverage changes

Copy link
Sponsor Member

@escopecz escopecz left a comment

Choose a reason for hiding this comment

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

I see no problem with the code changes 👍

@escopecz escopecz merged commit dd9f063 into mautic:5.x Apr 19, 2024
15 of 16 checks passed
@escopecz escopecz added this to the 5.1.0 milestone Apr 19, 2024
@escopecz escopecz removed the pending-test-confirmation PR's that require one test before they can be merged label Apr 19, 2024
@andersonjeccel
Copy link
Contributor Author

@RCheesley I meant, all these listed images show header with the old location for the notifications icon (even when they’re not the focus)

but one image, “notifications” needs to be changed since it’s about where the user can click to find the panel

I'd like to know if we could leave the others (unrelated to notifications) as they are since capturing dozens of new images would take a long time to replace

@RCheesley
Copy link
Sponsor Member

Only need to update the images which are impacted by the change.

@RCheesley
Copy link
Sponsor Member

@andersonjeccel just a reminder that the docs are needed for this PR.

@andersonjeccel
Copy link
Contributor Author

Had on my to-do list
Done!

@andersonjeccel andersonjeccel deleted the ui-Change-all-header-icons branch May 1, 2024 15:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
code-review-passed PRs which have passed code review enhancement Any improvement to an existing feature or functionality T1 Low difficulty to fix (issue) or test (PR) user-experience Anything related to related to workflows, feedback, and navigation user-interface Anything related to appearance, layout, and interactivity
Projects
Archived in project
Status: Done
Development

Successfully merging this pull request may close these issues.

6 participants