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] Remove borders from remaining tables #13748

Open
wants to merge 4 commits into
base: 5.x
Choose a base branch
from

Conversation

andersonjeccel
Copy link
Contributor

@andersonjeccel andersonjeccel commented May 14, 2024

Q A
Bug fix? (use the a.b branch) [ ]
New feature/enhancement? (use the a.x branch) [x]
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:

This PR removes borders and stripes from tables, similar to the other PR regarding this topic, but now standardizing the entire interface.

Before:
image

After:

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. API Credentials > List
  3. Assets > Create new > Save & Close > Details
  4. Channels > Dynamic Content > New > Save & Close > Details
  5. Campaigns > New > Save & Close > Details
  6. Emails > New > Save & Close > Details
  7. Forms > New > Save & Close > Details
  8. Companies > New > Save & Close > Details
  9. Contacts > Import > Select a file and import > Import history > Click on the import > Details
  10. Contacts > New > Save & Close > Details
  11. Segments > New > Save & Close > Details
  12. Marketplace > Any plugin > See those 2 tables
  13. Plugins > OneSignal > Fill all fields with anything > Save & Close > Channels > Mobile Notif > New > Save & Close > Details
  14. Channels > Web Notif > New > Save & Close > Details
  15. Components > Landing pages > New > Save & Close > Details
  16. Reports > Open any > Details
  17. Plugins > Twilio > Fill anything and set to Active > Save & Close > Channels > Text Messages > New > Save & Close > Details
  18. Channels > Focus > New > Builder > Select any style and type > Save & Close > Details
  19. Plugins > Twitter > Fill anything and set to Active > Save & Close > Channels > Social Monitoring > New > Save & Close > Details
  20. Channels > Mobile Notif > See the list
  21. Components > Forms > Click to see a form result (you need to have one created) > See the list

There are many other tables, but they're all the same situation as the above.

@andersonjeccel
Copy link
Contributor Author

andersonjeccel commented May 14, 2024

@escopecz Is testing necessary for this one? I mean, we would have about 48 different steps to see the same thing: tables without vertical lines

@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 enhancement Any improvement to an existing feature or functionality labels May 14, 2024
@andersonjeccel andersonjeccel self-assigned this May 14, 2024
@andersonjeccel andersonjeccel requested review from a team, ricfreire and Esthertests May 14, 2024 14:21
@escopecz
Copy link
Sponsor Member

@andersonjeccel it's always good to let testers to click through at least some places. It doesn't have to be dozens of places.

@andersonjeccel andersonjeccel added the ready-to-test PR's that are ready to test label May 15, 2024
@LordRembo
Copy link
Contributor

@Mike-Dropsolid and @andersonjeccel Any specific reasoning behind getting rid of the striped tables? Maybe I missed in in the Slack discussions. Either way, code-wise it looks fine.

Copy link

@PatrickJenkner PatrickJenkner left a comment

Choose a reason for hiding this comment

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

I just tested the PR, and most of the places I checked looked good. However, the table for the contact history still seems to use stripes (see the top 3 rows in the screenshot). Is that intentional?
Also, apparently the row "contact identified" still has borders between the columns while the others do not.
PR13748

@andersonjeccel
Copy link
Contributor Author

@LordRembo Norman (from webmecanik) suggested to make tables look like the Stripe platform on Slack: no background or column lines

people liked it, minimalist

I released a PR to change the main pages
It was merged some weeks ago

this one just extends the standard across everywhere

@andersonjeccel
Copy link
Contributor Author

@PatrickJenkner Hm, probably there’s another system happening to append colors to specific rows based on the events

I’ll check where it’s coming from

Copy link

codecov bot commented May 31, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 61.93%. Comparing base (4883fa0) to head (cf61b1c).
Report is 17 commits behind head on 5.x.

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff              @@
##                5.x   #13748      +/-   ##
============================================
+ Coverage     61.62%   61.93%   +0.30%     
- Complexity    34145    34196      +51     
============================================
  Files          2245     2249       +4     
  Lines        102077   102267     +190     
============================================
+ Hits          62909    63336     +427     
+ Misses        39168    38931     -237     

see 23 files with indirect coverage changes

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
code-review-needed PR's that require a code review before merging enhancement Any improvement to an existing feature or functionality ready-to-test PR's that are ready to test T1 Low difficulty to fix (issue) or test (PR) user-interface Anything related to appearance, layout, and interactivity
Projects
Status: 🦸🏻 Needs 2 tests
Status: Done
Development

Successfully merging this pull request may close these issues.

None yet

4 participants