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

Closes #898 Update close icon on upsell banners and admin bar #901

Merged
merged 6 commits into from
Oct 4, 2024

Conversation

Tabrisrp
Copy link
Contributor

Description

Fixes #898

Update close icon to use default icon from WP

Type of change

  • Enhancement (non-breaking change which improves an existing functionality).

Detailed scenario

  • With quota < 20%
  • Display upsell banner on settings or bulk optimization page
  • Display admin bar Imagify sub-menu
  • Close icon is updated to default icon used in WP and in white

admin-bar
bulk
settings

Technical description

Documentation

Updated class used + CSS color

Mandatory Checklist

Code validation

  • I validated all the Acceptance Criteria. If possible, provide screenshots or videos.
  • I triggered all changed lines of code at least once without new errors/warnings/notices.

Code style

  • I wrote a self-explanatory code about what it does.
  • I did not introduce unnecessary complexity.

@Tabrisrp Tabrisrp self-assigned this Sep 26, 2024
@Tabrisrp Tabrisrp requested a review from a team September 26, 2024 18:34
@Tabrisrp Tabrisrp added this to the 2.2.3 milestone Sep 26, 2024
@@ -35,7 +35,7 @@
<?php endif; ?>
<p><?php echo $data['text']; ?></p>
<p class="center txt-center text-center"><a class="imagify-upsell-admin-bar-button" href="<?php echo esc_url( $data['upgrade_link'] ); ?>" target="_blank"><?php echo $data['button_text']; ?></a></p>
<a href="<?php echo esc_url( get_imagify_admin_url( 'dismiss-notice', 'upsell-admin-bar' ) ); ?>" class="imagify-notice-dismiss imagify-upsell-dismiss" title="<?php esc_attr_e( 'Dismiss this notice', 'imagify' ); ?>"><span class="screen-reader-text"><?php esc_html_e( 'Dismiss this notice', 'imagify' ); ?></span></a>
<a href="<?php echo esc_url( get_imagify_admin_url( 'dismiss-notice', 'upsell-admin-bar' ) ); ?>" class="imagify-notice-dismiss imagify-upsell-dismiss notice-dismiss" title="<?php esc_attr_e( 'Dismiss this notice', 'imagify' ); ?>"><span class="screen-reader-text"><?php esc_html_e( 'Dismiss this notice', 'imagify' ); ?></span></a>
Copy link

Choose a reason for hiding this comment

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

@Tabrisrp Is there a reason that we didn't remove imagify-upsell-dismiss here while we removed it in part-upsell.php file?

@Mai-Saad
Copy link

Mai-Saad commented Oct 1, 2024

@Tabrisrp Thanks for the PR.
1- settings page ✔️
settingsPage2
2- Bulk page => not sure why icon is grey + the one in the page banner has underline
inBulkOPtimization
3- admin bar from WP dashboard page
AnyWPAdminPage
4- the notice when click bulk optimization link while having no credit (not mentioned in main ticket but just here for reference)
WhenClickBulkOPtimizationLink

@Tabrisrp Any idea why 2,3 has the icon grey and may be underlined?

@Tabrisrp
Copy link
Contributor Author

Tabrisrp commented Oct 1, 2024

I pushed an update for the CSS for cases 2 and 3.

@Mai-Saad
Copy link

Mai-Saad commented Oct 4, 2024

Thanks, @Tabrisrp Looks good now.
Screenshot from 2024-10-04 12-15-34
Screenshot from 2024-10-04 12-15-45
Screenshot from 2024-10-04 12-15-23

@Tabrisrp Tabrisrp added this pull request to the merge queue Oct 4, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to a conflict with the base branch Oct 4, 2024
@Tabrisrp Tabrisrp added this pull request to the merge queue Oct 4, 2024
Merged via the queue into develop with commit 56b4c97 Oct 4, 2024
6 checks passed
@Tabrisrp Tabrisrp deleted the enhancement/898-update-banners branch October 4, 2024 16:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update upsell banners and notification
3 participants