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

task/WP-355: Fixing issue with icons on dev/prod sites #892

Merged
merged 3 commits into from
Nov 1, 2023

Conversation

tjgrafft
Copy link
Collaborator

@tjgrafft tjgrafft commented Nov 1, 2023

Overview

During the testing session, we realized that all app icons in the categories are defaulting to icon-applications instead of their unique icon or the category icon on dev.cep and frontera (dev) sites. The previous PR WP-273 was working correctly on cep.test, however once deployed to dev/pprod, the logic failed. Wanted to investigate where there was a difference between local env and dev/pprod env.

Portals: cep, frontera.

Related

Changes

  • In the doesClassExist function, added a second check in the if statement to see if either ::before or :before pseudos existed after the className.
  • In the stylesheet/css files we were using to check for classNames--all the css rules have ::before after the icon classNames. However, once the css file was being built for dev/prod the css rules were changing to :before after each icon className in the file. This was fine for cep.test and testing locally--but in dev.cep and frontera it was failing bc it was checking for classNames with ::before only. (Look at screenshots below for context)
  • Added a doesClassExist.test.js file to run various unit tests on the function.

Testing

  1. You can test it locally on cep.test first and make sure the icons in the Applications tray either have their own unique icon, have their category's icon (if no unique icon for that app), or defaults to icon-applications (if no unique icon for app and category doesn't have its own custom icon either).
  2. The real test will be checking dev.cep and frontera (dev) once this branch is deployed there to see if the local env and dev/pprod env are consistent for this feature in both places--with the new code.

UI

Tested this in the console on dev.cep to make sure it would work correctly using the stylesheet available there via fetch.
This is an example of how the code was written before where it was checking for ::before only (notice function returns false)
Original

This is an example of me manually changing the doesClassExist function to check for :before instead (now the function returns true)
New

Combining the two checks in the if-statement to make sure both cases are handled regardless of how css file/rules are being built in dev/pprod (still returns true)
Logic_for_dev

Notes

Copy link

codecov bot commented Nov 1, 2023

Codecov Report

Merging #892 (530c8e8) into main (2254481) will increase coverage by 0.03%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #892      +/-   ##
==========================================
+ Coverage   63.40%   63.44%   +0.03%     
==========================================
  Files         432      432              
  Lines       12381    12383       +2     
  Branches     2574     2576       +2     
==========================================
+ Hits         7850     7856       +6     
+ Misses       4319     4317       -2     
+ Partials      212      210       -2     
Flag Coverage Δ
javascript 69.76% <100.00%> (+0.07%) ⬆️
unittests 56.99% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
client/src/utils/doesClassExist.js 100.00% <100.00%> (+44.44%) ⬆️

Copy link
Member

@rstijerina rstijerina left a comment

Choose a reason for hiding this comment

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

LGTM!

@rstijerina rstijerina merged commit b51c41f into main Nov 1, 2023
6 checks passed
@rstijerina rstijerina deleted the task/WP-355-app-category-dev-fix branch November 1, 2023 19:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants