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

Reworked display of product alerts #3303

Closed
wants to merge 11 commits into from
Closed

Reworked display of product alerts #3303

wants to merge 11 commits into from

Conversation

luigifab
Copy link
Contributor

@luigifab luigifab commented Jun 5, 2023

Description

Same but different :)

for configuration (system > configuration > catalog):

cnf-before

cnf-after

for product view:

prd-before

prd-after

Contribution checklist

  • Pull request has a meaningful description of its purpose
  • All commits are accompanied by meaningful commit messages
  • All automated tests passed successfully (all builds are green)
  • Add yourself to contributors list

@github-actions github-actions bot added Component: Adminhtml Relates to Mage_Adminhtml Component: Catalog Relates to Mage_Catalog Component: ProductAlert Relates to Mage_ProductAlert Template : admin Relates to admin template translations Relates to app/locale labels Jun 5, 2023
Copy link
Contributor

@kiatng kiatng left a comment

Choose a reason for hiding this comment

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

I like it. But how about retaining the original labels, which are more explanatory?

app/code/core/Mage/ProductAlert/etc/system.xml Outdated Show resolved Hide resolved
app/code/core/Mage/ProductAlert/etc/system.xml Outdated Show resolved Hide resolved
@S0FTWEX
Copy link
Contributor

S0FTWEX commented Jun 7, 2023

@luigifab, I would like to keep the original labels too (they are more explanatory). Or better: you can move them to - this is the best solution.

@luigifab
Copy link
Contributor Author

luigifab commented Jun 8, 2023

I'm going to update soon.

@addison74
Copy link
Contributor

addison74 commented Jul 13, 2023

I agree with this PR in the following design

1. System > Configuration > Catalog

screenshot_1

Comment: we are in the "Product Alerts" section, the two sub-sections must be "Price" and "Back in Stock", as obvious and simple as possible, without abusing of words. The rest of the phrases for every field remained the initial ones.

2. Product Edit page > Product Alerts tab

screenshot_2

Comments: Initial phrases "Price alert subscription was saved." and "Stock notification was saved." they are devoid of logic. Those grids are just a listing of alerts, but they can't be edited to talk about a save.

I would rename them as "Price" and "Back in Stock" since we are in the "Price Alerts" tab, again without abusing of words. We can reformulate them "Price Notifications" and "Back in Stock Notifications" as well or "Price Subscriptions" and "Back in Stock Subscriptions".

@addison74
Copy link
Contributor

addison74 commented Jul 13, 2023

A few personal opinions:

1 - I agree with the removal of the <div> container containing h4 - "Product Alerts" from the /product/tab/alert.phtml file. Its display is illogical and does not comply with the Backend presentation rules.

2 - I do not agree with the two <div> containers inserted in the /Tab/Alerts.php file. I would like to keep the presentation rules as before, if we make this change it would mean making changes in other places as well. Changing the titles is enough.

@fballiano
Copy link
Contributor

tested again today, I like this PR but I too would remove the div around the table as @addison74 says.

fballiano
fballiano previously approved these changes Jul 14, 2023
Copy link
Contributor

@fballiano fballiano left a comment

Choose a reason for hiding this comment

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

I've removed those divs around the table and IMHO it could be merged now :-)

@addison74
Copy link
Contributor

@fballiano - Let's not rush because it's not finished yet. Please take a look here #3303 (comment). I made important observations after testing that must be implemented.

@addison74 addison74 self-assigned this Jul 14, 2023
@luigifab luigifab closed this by deleting the head repository Jul 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component: Adminhtml Relates to Mage_Adminhtml Component: Catalog Relates to Mage_Catalog Component: ProductAlert Relates to Mage_ProductAlert Don't forget this PR Template : admin Relates to admin template translations Relates to app/locale
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants