refactor(970): moves notices for joining to its own component #972
Conversation
We were copying the same notice for joining in two places. This moves it to its own component, so future changes are easier.
ankushdharkar
force-pushed
the
970-update-homepage
branch
from
f1a7fc0 to
84ba5d2
Compare
Deploying www-rds with
|
| Latest commit: |
4249537
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://9c36b926.www-rds.pages.dev |
| Branch Preview URL: | https://970-update-homepage.www-rds.pages.dev |
|
Hi @ankushdharkar , I have a question. When creating a PR, should we strictly stay within the scope (e.g., just creating a reusable component) and leave related tasks like correcting the text for a separate PR? Or is it better to address both in one PR to make the feature complete? |
@AnujChhikara It depends. I like to keep things easier for reviewers. If I have to make new changes and refactor, I increase the scope of review here. Now, one group of reviewers just have to check if this PR creates no side-effects. And a future (and maybe different group) can check if the new message for 2025 is correct. Since each group has to focus on smaller changes, the review is less likely to get stuck and will be moved forward faster. |
ankushdharkar
changed the title
|
Can we please update the screenshot to reflect new changes ? I think attached screenshot is outdated. |
|
Please change the text from 'Closes' to 'Open' before GitHub issue ticket of Issue Details section as issue is still open. |
@surajmaity1I don't think we need that, Closes mean this PR resolve the issue and we follow the same format in our PRs also |
Good callout @surajmaity1 But this, is PR 1/N. If you take a look at the issue, there are more changes to be done after this refactoring PR is merged. In fact, this PR is going to make it easier to make those future changes correctly. |
It makes sense. It'll be better if you break the issue into few parts and mention in this ticket ( #970 ) with checkboxes and mark only checkboxes those are completed so that we can track whole task. |
That may be an overkill here, since PR description should be complete on its own. Happy to discuss if having atomic issues is mandatory, or a suggestion.
|
|
Suggestion: I see this sentence |
Given that the code for 2 sentences are going to be deleted during feature flag cleanup, and the sentence will then live on only in the single component, putting it in a constant may be unnecessary at this point, as a single source of truth will be maintained i.e |
Can we include a note in the 'Additional Notes' section clarifying that this PR doesn't completely close the issue? |
* [My Site Migration] Identity Linking get started page (#965) * resolve merge commits * feat: add get started page * remove: unused auth url * fix: css for get started on identity page * feat: add identity page behind feature flag * nit: remove unwanted .DS_store --------- Co-authored-by: Mehul Kiran Chaudhari <[email protected]> * [My Site Migration] Identity Linking step1 page (#966) * resolve merge commits * feat: add get started page * remove: unused auth url * fix: css for get started on identity page * feat: add identity page behind feature flag * feat: setp1 page in identity linking * remove: unused redirect-auth util * Remove .DS_Store files from repository * feat: add varaible css * feat: add varaible css * refactor(970): moves notices for joining to its own component (#972) * feat(970): moves notices for joining to its own component We were copying the same notice for joining in two places. This moves it to its own component, so future changes are easier. * refactor(970): added review changes: year - 2024 * refactor(970): fixed messaging for joining --------- Co-authored-by: Amit Prakash <[email protected]> Co-authored-by: Achintya Chatterjee <[email protected]> * [My Site Migration] Identity Linking step2,3 page (#967) * resolve merge commits * feat: add get started page * remove: unused auth url * fix: css for get started on identity page * feat: add identity page behind feature flag * feat: setp1 page in identity linking * remove: unused redirect-auth util * Remove .DS_Store files from repository * feat: add varaible css * feat: add varaible css * feat: add step2 on identity linking page * feat: add step2 and step3 page * feat: add reload step * feat: add varaible css --------- Co-authored-by: TEJAS <[email protected]> Co-authored-by: Mehul Kiran Chaudhari <[email protected]> Co-authored-by: Ankush Dharkar <[email protected]> Co-authored-by: Amit Prakash <[email protected]> Co-authored-by: Lakshay Manchanda <[email protected]>
We were copying the same notice for joining in two places. This moves it to its own component, so future changes are easier.
Date: Jan 5, 2025
Developer Name: Ankush Dharkar
Issue Details
Description:
We want to update the text copy for messaging on how to join our squad. But noticed that the message is duplicated, and currently doesn't have support to have some more html added to redirect links and such. E.g a link to follow to twitter flowchart to join, etc.
This change prepares the component that will be reused in future to display the messaging regarding how to join RDS
Is Under Feature Flag
Database changes
Breaking changes (If your feature is breaking/missing something please mention pending tickets)
Is Development Tested?
Tested in staging?
Add relevant Screenshot below ( e.g test coverage etc. )