-
Notifications
You must be signed in to change notification settings - Fork 0
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
Podcast Planner verbiage and layout #628
Conversation
<div class="mb-4"> | ||
<h3 class="fw-bold"><%= t(".header.day_select") %></h3> | ||
<p><%= t(".help.day_select") %></p> | ||
<h3 class="mb-3"><%= t(".header.day_select") %></h3> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Example of simplifying the text presented to the user.
<%= form.date_field :start_date, min: DateTime.tomorrow, max: DateTime.tomorrow + 1.years, value: DateTime.tomorrow, data: {action: "click#submit"} %> | ||
<%= form.label :start_date %> | ||
</div> | ||
</div> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I adjusted the layout of this field so that the selector field is full width, with two variable fields appearing below. This is more consistent with our UI patterns.
@@ -8,7 +8,7 @@ | |||
} | |||
) do |form| %> | |||
<h1 class="text-black fw-bold d-flex flex-fill"><%= t(".header") %></h1> | |||
<p><%= t(".help_html", url: new_podcast_episode_path(@podcast)) %></p> | |||
<p class="lh-sm"><%= t(".help_html", url: new_podcast_episode_path(@podcast)) %></p> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Line-height was really tall, looked weird.
<%= form.select :publish_time, time_options %> | ||
<%= form.label :publish_time %> | ||
</div> | ||
</div> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This file was reworked since its no longer technically a sidebar card
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Functionally there shouldn't be any different. All presentational here.
@@ -230,13 +230,13 @@ en: | |||
success: Episode drafts generated. | |||
form_main: | |||
header: Plan Episodes | |||
help_html: Save yourself some time by automatically creating multiple episode drafts to create a production schedule, it may also help your revenue as our sales teams will have access to dropdates to sell against. If you only wish to add a single episode, <a href="%{url}">go here</a>. | |||
help_html: Add multiple episode drafts to create a production schedule. Episode drafts improve the accuracy of your podcast's forecasts. If you only need to add a single episode, <a href="%{url}">go here</a>. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A verbiage review in this file is very appreciated!!!
<div class="card p-4 mt-4mb-4"> | ||
<div data-controller="pager" data-pager-range-value="6"> | ||
<div class="d-flex justify-content-between"> | ||
<h3 class="mb-3"><%= t(".header.number_of_drafts") %></h3> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this needs a count
variable to pass through to the yml file in order to display the correct number.
you can probably pass through @planner.dates.count
but that would only reflect the dates generated through the calculator, not selected manually. i could probably figure out a helper method to add to the stimulus controller that would update every time a manual date has been clicked as well. is this an important feature for parity?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in general i have no comments on verbiage since this is a return to parity verbiage and we're not planning on the improvements until post-launch. the only comment i made was regarding the count
variable for that one header label. the only other comment i would make is figuring out a color key since that is one thing new to the calendar.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should have put previous comment under request changes for the count
variable.
@@ -5,6 +5,7 @@ module PodcastPlannerHelper | |||
MONTHLY_WEEKS = I18n.t([:first, :second, :third, :fourth, :fifth], scope: [:podcast_planner, :helper, :monthly_options]) | |||
DATE_CONTROLLER = "date" | |||
TOGGLE_ACTION = "click->date#toggleSelect" | |||
RECOUNT_ACTION = "click->count#recount" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this and the counterTarget
below are additions for the new stimulus count_controller
. Essentially adds a counter to each date. the controller counts the ones that are selected.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
moved counterTarget
to the input in the _calendar
partial.
publish_time: What time of day do you plan to publish the episodes? | ||
audio_segments: How many audio segments? | ||
segment_count: Number of segments | ||
number_of_drafts: "Drafts planned: " |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
changed this so it no longer needs to pass in a count
variable. the count is displayed in the view in the innerHTML of a new span in the h3 that uses this label.
<%= form.label :segment_count %> | ||
</div> | ||
<div class="card p-4 mt-4mb-4"> | ||
<%= turbo_frame_tag "preview", target: "_top" do %> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
moved turbo_frame up here so that the count display will update when the calculator changes selections.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Couple minor suggestions @radical-ube. But all pretty much your call - they're slightly stylistic.
@brandonhundt one slight issue on wide viewports
It isn't going to be trivial to display 8 calendars magically as the user widens the viewport. So may just want to restrict the max-width of those for now? Or force maximum 3-per-row?
const selectedTargets = this.counterTargets.filter((el) => { | ||
return el.firstElementChild.disabled | ||
}) | ||
return this.counterTargets.length - selectedTargets.length |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Naming is a bit misleading ... selectedTargets
appears to be all the deselected hidden fields? Maybe invert the above filter to !disabled
, and then just return .length
?
export default class extends Controller { | ||
static targets = ["counter", "display"] | ||
static values = { | ||
selected: { type: Number, default: 0 }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this even being passed in anywhere, or are you getting the default?
I'd probably suggest omitting this entirely. The server should render the correct number in the HTML to start with. And then drop the connect()
method here, and just wait until some action calls recount()
to have this controller modify the innerHTML.
color: $light; | ||
cursor: pointer; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
to let the user know they can click to add dates. chose the link color to emphasize clicking.
@@ -1,9 +1,9 @@ | |||
<div class="col <%= "d-none" if index >= range %>" data-pager-target="page"> | |||
<div class="col col-xl-4 <%= "d-none" if index >= range %>" data-pager-target="page"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thats all it was to fix the grid issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
closes: #606
Now that we have a working calendar (!!!EXCITING!!!) I thought we could take a pass at the verbiage and the layout of the calendar. This PR's changes will get us closer to what exists in Publish currently where we've received a lot of neutral to positive feedback. (NOTE: I think we can open a new ticket to explore enhancements to this feature that go beyond the existing functionality in this PR.)
There were a lot of places where we had help text, headers, and field labels that felt a little redundant or overly technical in its use, so I attempted to clean It up.
Also, I reverted this to a single column layout like we have now. I think this will serve our producers finely enough until we get the time to really rethink the feature in the future.