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

Improve naming consistency of delivery options #206

Closed
raamdev opened this Issue Jan 11, 2016 · 24 comments

Comments

Projects
None yet
5 participants
@raamdev
Contributor

raamdev commented Jan 11, 2016

@RealDavidoff writes in #199 (comment)

  1. "Delivery option" - once you say "asap", then you say "instantaneous". Why use different words for the same thing?
@RealDavidoff

This comment has been minimized.

RealDavidoff commented Jan 14, 2016

Raam, there is a related bug, as follows:
raam-error1
raam-error2
raam-error3

@RealDavidoff

This comment has been minimized.

RealDavidoff commented Jan 14, 2016

you can also see the mini text-size there. solution would be as described earlier: define it, to make sure.

@raamdev

This comment has been minimized.

Contributor

raamdev commented Jan 14, 2016

@RealDavidoff I'm not able to reproduce that bug. Where are you testing that? I just tried it on a regular comment form on my test site and changing to "no, do not subscribe" and then back to "yes, all comments/replies" had no effect on the delivery options box... it still works as expected.

@RealDavidoff

This comment has been minimized.

RealDavidoff commented Jan 14, 2016

It's because you didn't start off at the SETTINGS page ;-)
We wanted default: No, do not subscribe... Try it!

@raamdev

This comment has been minimized.

Contributor

raamdev commented Jan 14, 2016

@RealDavidoff I'm still not able to reproduce any problem.

With my Settings configured with "No, do not subscribe" as the default:

2016-01-14_10-11-45

All of the options on the Comment Form work properly:

2016-01-14_10-12-12
2016-01-14_10-12-19
2016-01-14_10-12-26

@jaswrks

This comment has been minimized.

Member

jaswrks commented Jan 25, 2016

@raamdev Which do we prefer?

  • asap
  • instantly

I'll vote for instantly, since it is a word that is more easily translated than the asap acronym.

@jaswrks

This comment has been minimized.

Member

jaswrks commented Jan 25, 2016

@kristineds This looks like an issue you could try to outline once Raam replies back.

@raamdev

This comment has been minimized.

Contributor

raamdev commented Jan 25, 2016

Yes, I agree that instantly is the better option.

@jaswrks

This comment has been minimized.

Member

jaswrks commented Jan 26, 2016

Next Actions

  • New feature branch in the websharks/comment-mail-pro repo.
  • Search the entire codebase for the word: asap: https://github.com/websharks/comment-mail-pro/search?p=1&q=asap&utf8=%E2%9C%93
  • Note that there are two different ways this is used in the codebase.
    • In one form, it is used as a hard-coded identifier that assigns a specific delivery type in the DB.
    • In the second form, there are some places where asap is used in the UI or in translation routines that result in a GUI presentation of the word asap.
      • It is the second form that you should work to update. We want to use the word instantly instead of the acronym asap.
  • Submit PR for review.

Once approved, repeat in the lite version of the software.


Examples of a hard-coded delivery type status code that should NOT change.

Examples of a place where the word asap is used in HTML markup that is presented in a GUI.

@jaswrks

This comment has been minimized.

Member

jaswrks commented Jan 26, 2016

@kristineds I outlined this for Renz. Maybe you can review his PR :-)

@kristineds

This comment has been minimized.

Contributor

kristineds commented Jan 26, 2016

Maybe you can review his PR :-)

Sure thing! I'll just wait for @renzms to submit it . :)

@renzms

This comment has been minimized.

Contributor

renzms commented Jan 26, 2016

@kristineds
cc: @jaswsinc

Ready for check! Thanks!

@kristineds

This comment has been minimized.

Contributor

kristineds commented Jan 28, 2016

@renzms Quoting what Jason said regarding POT translation files. :)

FYI: You don't need to edit anything in the POT translation file. That file is auto-generated before each release of the software. So any edits that you make there will get wiped away anyway. No harm in editing the file, but just so you know. Don't worry about that file. Saves time.


@jaswsinc: I'm seeing these files that needs to be updated as well, i.e. change asap to instantly. Could you please check ? :) cc @raamdev

Template File Link to Codebase
/comment-mail-pro/includes/templates/type-a/site/comment-form/sub-ops.php https://github.com/websharks/comment-mail-pro/blob/28bbe8b88b59a77e3632938d5c72314ff86488a2/comment-mail-pro/includes/templates/type-a/site/comment-form/sub-ops.php#L61
https://github.com/websharks/comment-mail-pro/blob/28bbe8b88b59a77e3632938d5c72314ff86488a2/comment-mail-pro/includes/templates/type-a/site/comment-form/sub-ops.php#L59
/comment-mail-pro/includes/templates/type-s/site/comment-form/sub-ops.php https://github.com/websharks/comment-mail-pro/blob/78ccc101b44deba820bea28875f3f567b45baea6/comment-mail-pro/includes/templates/type-s/site/comment-form/sub-ops.php#L100
/comment-mail-pro/includes/classes/utils-markup.php https://github.com/websharks/comment-mail-pro/blob/c466208f9a8fe0d824501795ca2afc32f60b11fd/comment-mail-pro/includes/classes/utils-markup.php#L571
@jaswrks

This comment has been minimized.

Member

jaswrks commented Jan 29, 2016

https://github.com/websharks/comment-mail-pro/blob/28bbe8b88b59a77e3632938d5c72314ff86488a2/comment-mail-pro/includes/templates/type-a/site/comment-form/sub-ops.php#L61

This shows instantly, even though the hard-coded DB value is asap. It's OK to leave this as-is.


https://github.com/websharks/comment-mail-pro/blob/28bbe8b88b59a77e3632938d5c72314ff86488a2/comment-mail-pro/includes/templates/type-a/site/comment-form/sub-ops.php#L59

This is just a code comment, and it references the hard-coded DB value. So you can leave this as-is.


https://github.com/websharks/comment-mail-pro/blob/78ccc101b44deba820bea28875f3f567b45baea6/comment-mail-pro/includes/templates/type-s/site/comment-form/sub-ops.php#L100

This shows instantly, even though the hard-coded DB value is asap. It's OK to leave this as-is.


https://github.com/websharks/comment-mail-pro/blob/c466208f9a8fe0d824501795ca2afc32f60b11fd/comment-mail-pro/includes/classes/utils-markup.php#L571

$this->plugin->utils_i18n->deliver_label('asap')

Once this PR is merged, that translation utility should automatically convert asap into instantly.


The word asap is used internally, and it's one of the enumerable values in the DB for delivery type. So that term is still going to be a part of the codebase. Only the UI representation of that value is changing, so I think those places you pointed out are OK to leave as-is.

@kristineds Have you run this branch through any tests inside WordPress? Were any of the issues you noted above causing problems from the UI perspective whenever you tested?

@jaswrks

This comment has been minimized.

Member

jaswrks commented Jan 29, 2016

@kristineds It might help if you ask Renz to take some screenshots to make reviewing a little easier.

@raamdev

This comment has been minimized.

Contributor

raamdev commented Feb 1, 2016

@kristineds This is still waiting for a review from you. See also the PR Renz submitted websharks/comment-mail-pro#50.

@renzms Also, noting here that we'll need a PR for the Lite version as well. When a PR will be needed for both versions, it's best to always work on those side-by-side, i.e., to make the changes at the same time for both the Lite and the Pro version, otherwise it becomes easy to lose track of what changed where. 😄

@kristineds

This comment has been minimized.

Contributor

kristineds commented Feb 2, 2016

@raamdev I've posted my review here. :) websharks/comment-mail-pro#50 (comment)

@raamdev I reviewed it and @jaswsinc point out some changes that still need to be made. (websharks/comment-mail-pro#50 (comment)) I'm listing down the files that needs to be updated by Renz.

@renzms Below are the list of the files that needs to be updated, i.e. change asap to instantly from the UI perspective. Most of them are notes on the menu section. Please include screenshots to make reviewing easier. :)

@renzms

This comment has been minimized.

Contributor

renzms commented Feb 3, 2016

@raamdev

When a PR will be needed for both versions, it's best to always work on those side-by-side, i.e., to make the changes at the same time for both the Lite and the Pro version, otherwise it becomes easy to lose track of what changed where.

Ok, will do. Just following the instructions in order of what @jaswsinc laid out earlier above:

Submit PR for review.
Once approved, repeat in the Lite version of the software.

Updating now and working on Lite version, thanks @kristineds

renzms added a commit to websharks/comment-mail-pro that referenced this issue Feb 3, 2016

@renzms

This comment has been minimized.

Contributor

renzms commented Feb 3, 2016

@raamdev @jaswsinc

There were 6 additional instances of asap besides the ones Kristine noted, I removed those as well as the line about (aka: instantly), as it would no longer make sense once the word was replaced with instantly. cc @kristineds

comment mail config options renz s test area wordpress 5

comment mail config options renz s test area wordpress 4

comment mail config options renz s test area wordpress 6

renzms added a commit that referenced this issue Feb 3, 2016

@raamdev

This comment has been minimized.

Contributor

raamdev commented Feb 3, 2016

Note that there are two different ways this is used in the codebase.

In one form, it is used as a hard-coded identifier that assigns a specific delivery type in the DB.

@jaswsinc Is the only reason we're not changing all uses of asap (including those used in the database) to instantly because that would require updating the database for existing Comment Mail installations when they upgrade?

It seems to me that changing all occurrences of asap to instantly and then creating a version upgrade routine that converts any existing uses in the database, would be better than continuing to have two ways of referring to the same thing (i.e., it adds to confusion in the code).

@jaswrks

This comment has been minimized.

Member

jaswrks commented Feb 3, 2016

@jaswsinc Is the only reason we're not changing all uses of asap (including those used in the database) to instantly because that would require updating the database for existing Comment Mail installations when they upgrade?

That is correct, yes.

@raamdev

This comment has been minimized.

Contributor

raamdev commented Feb 6, 2016

Next Lite Release Changelog:

  • Enhancement: Improved the consistency of how we refer to the "instant" delivery option by replacing any occurrences "asap" with "instantly". These names were previously being mixed. We now use "instantly". Props @RealDavidoff @renzms @kristineds. See Issue #206.
@raamdev

This comment has been minimized.

Contributor

raamdev commented Feb 6, 2016

Next Pro Release Changelog:

  • Enhancement: Improved the consistency of how we refer to the "instant" delivery option by replacing any occurrences "asap" with "instantly". These names were previously being mixed. We now use "instantly". Props @RealDavidoff @renzms @kristineds. See Issue #206.

@raamdev raamdev closed this Feb 6, 2016

@raamdev

This comment has been minimized.

Contributor

raamdev commented Feb 13, 2016

Comment Mail v160213 has been released and includes changes from this GitHub Issue. See the release announcement for further details.


This issue will now be locked to further updates. If you have something to add related to this GitHub Issue, please open a new GitHub Issue and reference this one (#206).

@websharks websharks locked and limited conversation to collaborators Feb 13, 2016

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.