feat: [MP-2477] - new next steps page final sync#6711
feat: [MP-2477] - new next steps page final sync#6711cristhianDt wants to merge 28 commits intomainfrom
Conversation
…ature/mp-2477-new-nextSteps-page-second-alternative
hide non badges slides in second section
|
Tested locally looking good |
|
I see this warn in console: |
| :accepted="acceptedEmailMarketingUpdates" | ||
| :loans="loans" | ||
| :latest-loan="latestLoan" | ||
| @accept-email-updates="acceptedEmailMarketingUpdates = true" |
There was a problem hiding this comment.
Are we expecting some tracking here?
There was a problem hiding this comment.
We have, I'm not in front on my laptop but I think the recording is in the parent
There was a problem hiding this comment.
ok, the recording is located in the component look:
| }; | ||
| const showSurveyCard = computed(() => checkShowSurveyCard({ | ||
| showPostLendingNextStepsCards: props.showPostLendingNextStepsCards, | ||
| postLendingNextStepsEnable: props.postLendingNextStepsEnable, |
There was a problem hiding this comment.
This is probably no more needed.
There was a problem hiding this comment.
So we are not using this flags anymore, is that?
There was a problem hiding this comment.
Yeah, that feat is rolled out to all users now.
There was a problem hiding this comment.
ok, again, I would like this to be handled in a dedicated PR. I have already made many changes to match with main and make more changes that are not part of the requirement that could extend even more this ticket, I have to wait a week for other changes. let's leave this for a future task
| storeGoalPreferences: goalData.storeGoalPreferences, | ||
| loadGoalData: goalData.loadGoalData, | ||
| trackingCategory: 'portfolio', | ||
| goalsV2Enabled: props.goalsV2Enabled, |
There was a problem hiding this comment.
Probably no more needed.
There was a problem hiding this comment.
same answer (a ticket to clean up this project)
|
|
||
| <style lang="postcss" scoped> | ||
| .stats-wrapper { | ||
| height: auto; |
Hi @roger-in-kiva could share me a screenshot and the whole error by slack , thanks |
| loans: mockLoans, | ||
| }); | ||
|
|
||
| export const EmptyState = story({ |
| loans: mockLoans, | ||
| }); | ||
|
|
||
| export const AllRegionsLent = story({ |
There was a problem hiding this comment.
Do we ever show this in app? Because the UI doesn't really make sense. If it's not used, we can remove. But if it's used, we need to chat with design about making this more usable.
There was a problem hiding this comment.
Well it's true but right now it is a probability, so I will start a new thread in our channel about , I mean if I should hide the module if all the regions were lent or not, agree?
There was a problem hiding this comment.
What happened previously? We can replicate that behavior.
There was a problem hiding this comment.
for the component a blank space but in the UI, I mean, the page we are hiding the section look this line
ui/src/components/MyKiva/LendingStats.vue
Line 217 in 5e550b8
There was a problem hiding this comment.
Yeah, I think there's no hidden state for the map. Although,I would show the region map and only hide cards/components that have a logic to hide after an action or logic. E.g. survey card
| } | ||
| }, | ||
| { | ||
| path: '/mykiva/next-steps', |
There was a problem hiding this comment.
Please also create a flux repo PR to add this URL to dev (see other recent PRs for where/how to do this).
There was a problem hiding this comment.
Yeah I just want to have this PR ready, let me create the other for flux 👍
| */ | ||
| export const getUrlParamsFromString = url => { | ||
| const urlSplit = url.split('?'); | ||
| return urlSplit[1]; |
There was a problem hiding this comment.
Are we always guaranteed a value here or do we need a fallback?
There was a problem hiding this comment.
mmm I can added it but this is an old logic, just moved to this new util, let me check
|
|
||
| if (urlParams && urlParams.includes(REFER_FRIEND_MODAL_KEY) && modalHandlers.openSharingModal) { | ||
| const paramsSplit = urlParams.split('='); | ||
| if (paramsSplit && paramsSplit[1] === 'true') { |
There was a problem hiding this comment.
toLowerCase needed here?
There was a problem hiding this comment.
same answer like my prev comment, I will check the old logic for that
|
|
||
| const urlParams = getUrlParamsFromString(secondaryCtaUrl); | ||
|
|
||
| if (urlParams && urlParams.includes(JOURNEY_MODAL_KEY) && modalHandlers.updateJourney) { |
There was a problem hiding this comment.
Need to handle casing here?
| isTieredAchievementComplete, | ||
| getActiveTierData, | ||
| }); | ||
| return buildUniversalOrderedSlides({ |
There was a problem hiding this comment.
Do we ever need to use universal ordered slides? Most of this carousel is being moved to the new page.
There was a problem hiding this comment.
probably not, in fact, I have another idea in mind for those components, but making changes over changes has already made me dedicate more time, I mean, syncs, etc. If it's not a problem, I'd like to create a task for that a kind of “tech enhancement"
| * @param {Function} options.getActiveTierData - From useBadgeData | ||
| * @returns {Array} Achievement slide objects | ||
| */ | ||
| export const buildAchievementSlides = ({ |
There was a problem hiding this comment.
It seems like these two methods could live in the other util with the similar method buildUniversalOrderedSlides, if we indeed need both.
| import { optimizeContentfulUrl } from '#src/util/imageUtils'; | ||
| import { formatUiSetting } from '#src/util/contentfulUtils'; | ||
|
|
||
| export const getRichTextContent = slide => slide?.fields?.richText?.content ?? []; |
There was a problem hiding this comment.
Methods missing jsdoc comments like other util files in this changeset.
| * @param {number|null} params.slidesNumber - Max slides to return (null = no limit) | ||
| * @returns {Array} Ordered sequence of slide objects | ||
| */ | ||
|
|
There was a problem hiding this comment.
Remove extra newline.
| * @returns {Array} Ordered sequence of slide objects | ||
| */ | ||
|
|
||
| export const buildUniversalOrderedSlides = ({ |
There was a problem hiding this comment.
See other comment, I'm not convinced we need this method now that the new page is built.
| @@ -0,0 +1,255 @@ | |||
| /** | |||
There was a problem hiding this comment.
This file is name as a composable and in the composables folder, but it's actually a set of methods. Change to be a util file.
| } from '#src/util/myKivaUtils'; | ||
|
|
||
| let badgeDataInstance = null; | ||
| const getBadgeData = () => { |
There was a problem hiding this comment.
This pattern is confusing. It means that this file, which is a util, is putting an instance of a. composable in memory. As far as I'm aware, that's not the intended pattern for composables and could be tough to maintain long-term, since you'll have to look at this file and the components to know where the composables are being instantiated.
There was a problem hiding this comment.
the idea is have just 1 instance of badgeData and used cross the Util (renamed), do you have any other ideas to suggest?
| </WwwPage> | ||
| </template> | ||
|
|
||
| <script setup> |
There was a problem hiding this comment.
Why is this second in many ways identical page needed? Could we instead revert most of the changes to MyKivaPage and add a prop to that page that loads different content with the same data loaded?
There was a problem hiding this comment.
In other words the new content here could be in a new component like MyKivaPageContent but with the same data loading page parent.
There was a problem hiding this comment.
that would mean modifying LendingStats to extract the information from there as well, and how would it determine which content to load? using the router?
There was a problem hiding this comment.
You could add a prop to MyKivaPage at the router level that would render one of the two templates. That prop could then also be provided to LendingStats This reduce the need for a good portion of the refactor. it would also remove the need for the in-memory storage added to the util file. Thoughts?
There have been a significant number of bugs lately in the MyKiva progress, and while we can explore this level of a refactor, it will take lots and lots and lots of functional testing with several different types of users to validate the functionality.
| return (!userGoal.value || !userGoalAchieved.value) && !hideCompletedGoalCard.value; | ||
| }); | ||
|
|
||
| onMounted(async () => { |
There was a problem hiding this comment.
We load most of this data on SSR on MyKivaPage, and we could either do the same here in an options API component, or like the other comment, utilize the same data-loading parent and remove a lot of duplicate from this component (and simplify the utils greatly).
There was a problem hiding this comment.
Yes, this relates to the discussion we had on Slack regarding the repositories. My plan is to shift the approach to use the Composition API, which will make it easier to interpret, migrate, and standardize across other repositories. I also think it's important to abstract the data loading and processing from the UI or template itself. The trend of having one large file with everything makes it difficult to maintain, especially as the project continues to grow.
I'm happy to close this PR and open a new one with a different approach if needed. However, it would be valuable to have a clear and defined vision for the future of our frontend repositories, so we can establish a unified standard—and why not work toward consolidating into a single frontend project if that is the case
There was a problem hiding this comment.
I will work in a new alternative following your coments
There was a problem hiding this comment.
All good ideas, but we don't have the time or resources to do that level of tech debt refactor at this time in Kiva's life. Like mentioned in Slack, ui generally should remain Options API due to how preFetch works. There are instances where Composition API can be used, but it has to be components that are entirely FE.
There was a problem hiding this comment.
Also a quick note that if we do in fact need the level of refactor in this PR for the new page, we can keep exploring that, I just think it's worth investing a little time today to explore if a simpler option is valid here. And we generally don't use in-memory "state" outside of composables and Apollo, so that would need extensive testing.
There was a problem hiding this comment.
yeah totally, okey, I created a draft with that alternative last week because that was one of my ideas as well let me work on that, but as I told you I wanted to switch from the API Options to the Composition API in the parent where we are loading the data
There was a problem hiding this comment.
Components that load data often need SSR, so they will need to stay Options API. I know it's annoying to have two Vue repos with different paradigms, but just part of working at an old company with only a handful of FE engineers 😿. Thanks for the flexibility in trying different options for this changeset!
Another good thing to consider in the meantime is putting up a smaller PR with aspects like pulling out the region component. This PR being 3K lines is really tough to get solid code reviews on.
There was a problem hiding this comment.
In fact, my initial idea was to create at least two PRs to make it easier to review, but I didn't expect this ticket to take so long, depending on other changes, synchronizing with main, and again, it delayed me
There was a problem hiding this comment.
so in the end I created just one PR (not the best idea of course)
dyersituations
left a comment
dyersituations
left a comment
There was a problem hiding this comment.
Overall nice work here. I'm not against changing the general pattern, but please dig into my comments around sharing the parent. If that seemed like a decent path forward, a new prop could be added to the parent, most of the file reverted here, and many of the utils removed (including instantiating the composable in memory in a JS file rather than a component).
| this.goalModalHandlers.closeImpactInsightsModal(this.modalRefs); | ||
| }, | ||
| openGoalModal(event) { | ||
| this.isUpdatingGoal = event?.updating || false; |
There was a problem hiding this comment.
@roger-in-kiva I synced my branch again with your recent changes, could you please check this file and useMyKivaJourneyData https://github.com/kiva/ui/pull/6711/changes#diff-2e1ede34c2bfa402da0a0903e75e34e01c2fef011d22fa2e366e2135441e7c0c
thank you
1. remove an empty state story for MyKivaRegionExperience.stories.js 2. css rules
6 participants
MP-2477
Summary
Create a new
/mykiva/next-stepspage, changes:https://kiva.atlassian.net/browse/MP-2477
Evidences
New Page with goal set
new.page.next.steps.with.goal.mov
New page without goal and full cards enabled
https://drive.google.com/file/d/19LvNVHOnJtUMW-XSZzdnKLR6d-rtH9zp/view?usp=sharing
No affected existing MyKiva page
https://drive.google.com/file/d/1O37jMjtOE_KXHY-4zqx0JJMa2m3ubBjg/view?usp=sharing