Add Salary Range Field & JobSchema to WPJM #2633
Conversation
|
@fjorgemota @mikeyarce @onubrooks asking for a review on this as you worked last on the salary area :) |
|
Thanks for the PR @masteradhoc ! The code implementation looks good - but my concern is with the labelling of the fields:
Overall, I think it would be great to have the option for Min/Max salary. |
wp-job-manager-template.php
Outdated
| * @param string $currency | ||
| * @param string $unit | ||
| * @param string $after | ||
| */ | ||
| $job_salary = apply_filters( 'the_job_salary_message', $job_salary, $post, $before, $salary, $currency, $unit, $after ); | ||
| $job_salary = apply_filters( 'the_job_salary_message', $job_salary, $post, $before, $salary, $currency, $salarymax, $unit, $after ); |
There was a problem hiding this comment.
This might break existing integrations
There was a problem hiding this comment.
@thedebian what do you suggest?
There was a problem hiding this comment.
Good question, I don't have an empirical solution to that, it can be anything from adding the param after $after, to changing $salary to be a concatenation of $min . '-' . $max, if it's a salary range .
I'd prefer the second option, as $salary is documented as string, it wouldn't break the contract, @gkaragia do you have an idea ?
There was a problem hiding this comment.
I think appending it as an extra argument in the end is the safest.
If we don't add it as an extra argument and concatenate instead, this will break existing sites that use the $after function argument or specify a $unit. In these cases the job salary mind end up with weird values in the filter.
Quite open to that. Just didnt wanted to add another setting for this as its quite common to have some jobs with no range (f.e. intern/trainee) and some with range. How would you like this to be changed? i think also this could make sense:
|
|
@gkaragia @thedebian any option to get this reviewed again? :) |
|
Hey @masteradhoc, as @mikeyarce suggested above I think that we should have separate fields for the salary range and actual salary. I think that the most versatile approach would be to have all three fields available, Salary, Salary Min, and Salary Max. Then we don't show the Salary field if both Salary Min and Salary Max are defined. @mikeyarce and @yscik what do you think about that? |
|
@gkaragia sounds like an option for me. You would have to turn it on and loose all of your existing values or copy it upfront and then reenter it again. The solution like this is designed that it works even for users that are upgrading to ranges or only using them sometimes.. |
|
Hey @masteradhoc, I am not suggesting adding an option for this. After having some second thoughts we should do the following:
If you don't have the capacity to implement the above, let us know and we will consider implementing it in a future release. |
|
Just as an update @masteradhoc we will look into implementing this in the future. |
|
We have decided that we will rework this in the near future so switching this to draft. |
|
@thedebian Giannis said i should contact you about this PR. Any movement? Please feel free to make the edits needed to get this into the next version. |
Hey @masteradhoc, it's in our backlog, for now we can't provide an ETA for it though, we'll let you know when we have an acceptable solution for this feature. |
Fixes #2629
Changes Proposed in this Pull Request
Testing Instructions
Release Notes