-
Notifications
You must be signed in to change notification settings - Fork 48
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
Production tickets for the week of 12-5-22 #2117
Conversation
🚢 Here is the frontend staging link: 🚢 |
** 👋 Attention translators!! 👋 ** |
🚢 Here is the frontend staging link: 🚢 |
** 👋 Attention translators!! 👋 ** |
🚢 Here is the frontend staging link: 🚢 |
** 👋 Attention translators!! 👋 ** |
🚢 Here is the frontend staging link: 🚢 |
** 👋 Attention translators!! 👋 ** |
🚢 Here is the frontend staging link: 🚢 |
** 👋 Attention translators!! 👋 ** |
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 couple questions! I wouldn't block merging on any of them.
@@ -40,7 +40,7 @@ const PublicEngagementPage = ({location}: IPublicEngagementPageProps) => { | |||
</p> | |||
</Grid> | |||
<Grid desktop={{col: 4}}> | |||
{/* <PublicVideoBox isBeta={false}/> */} | |||
<PublicVideoBox isBeta={false} youTubeLink='https://www.youtube.com/watch?v=XwilQp3EXRQ'/> |
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 question: Why do you sometimes encode the links directly in the page and sometimes reference them from vars? Is it if the link appears on more than one 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.
Generally speaking, if all instantiations of a component will render the same way, then the contents (links in this case) can be placed in the component itself. In cases where the instantiation of a component should render differently (in this case the youTubeLinks), then one way to capture those differences is to use props (what you may be referring to as vars).
Does that answer the question?
Also totally open to hearing other ideas on this :)
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.
Was honestly purely curious.
|
||
</p> | ||
</div> | ||
<div | ||
class="desktop:grid-col-4" | ||
data-testid="grid" | ||
/> | ||
> | ||
<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.
Any reason you build your own summary box and didn't use the trussworks one? I say this as a person who just basically did same because I don't like the UX of using the trussworks ones.
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.
Actually, I did use Trusswork's SummaryBox components to build a wrapper component called PublicVideoBox. The PublicVideoBox component uses Trusswork's SummaryBox under the hood and allows me to re-use this wrapper. Since the design required two summary boxes that look very similar (except styling and content differences), the wrapper component was created to allow both instances met with a single component.
This is also part of a larger goal of mine to be able to create more versatile components.
Did that answer the question?
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.
Oh I looked at the wrong file, so I saw the markup. Thank you for the thorough answer.
} | ||
// if 3 | ||
// if 3-1 | ||
} else if ( |
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 don't understand why you need both of these two new elifs, since >=0 is also ==0, and tehy both call the same copy? Maybe I'm misreading one of the variables tho --- is it to handle percentTractTribal sometimes being a number? If so, would it maybe be simpler to just have the >=
version and explicitly cast with Number(percentTractTribal)
since that'll work if percentTractTribal is a number or a string?
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.
Nice catch @mattbowen-usds !! That should be >= 1!!
Fixing now!
🚢 Here is the frontend staging link: 🚢 |
** 👋 Attention translators!! 👋 ** |
QA pass |
🚢 PR Deployed! 🚢 |
This PR closes the following tickets:
Please QA when feasible, TY!
QA LINK