Skip to content
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.

Sign up for GitHub

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

Jump to bottom

last used indicator on login page fixed #850

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

last used indicator on login page fixed #850

wants to merge 2 commits into from

Conversation

sahiladit
Copy link

Fixes:#841

#what does this pr do:
Now the indicator now only pops up when user click on any of the login mode and not when the user enters for the first time.

@sahiladit sahiladit requested a review from mfts as a code owner October 12, 2024 19:14
@vercel Vercel
Copy link

vercel bot commented Oct 12, 2024

@sahiladit is attempting to deploy a commit to the mftsio Team on Vercel.

A member of the Team first needs to authorize it.

@github-actions GitHub Actions
Copy link

github-actions bot commented Oct 12, 2024 •
edited
Loading

All contributors have signed the CLA ✍️ ✅
Posted by the CLA Assistant Lite bot.

@mfts
Copy link
Owner

mfts commented Oct 13, 2024

@sahiladit could you please share a video what you mean? if the user hasn't logged in ever then of course there's no "last used".

@sahiladit
Copy link
Author

@sahiladit could you please share a video what you mean? if the user hasn't logged in ever then of course there's no "last used".

It fixes #841

@mfts
Copy link
Owner

mfts commented Oct 13, 2024

ok sounds good. please sign the CLA

@sahiladit
Copy link
Author

ok sounds good. please sign the CLA

Can you give me the steps to sign it? It's my first time contributing to an org..

@mfts
Copy link
Owner

mfts commented Oct 13, 2024

@sahiladit

Thank you for your submission, we really appreciate it. Like many open-source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution. You can sign the CLA by just posting a Pull Request Comment same as the below format.

I have read the CLA Document and I hereby sign the CLA

You can retrigger this bot by commenting recheck in this Pull Request. Posted by the CLA Assistant Lite bot.

@sahiladit
Copy link
Author

recheck

@sahiladit
Copy link
Author

I have read the CLA Document and I hereby sign the CLA

@sahiladit
Copy link
Author

recheck

@JeevaRamanathan
Copy link
Contributor

JeevaRamanathan commented Oct 13, 2024 •
edited
Loading

Hi @sahiladit, It's great to hear on your first contribution, many more to go! One small thing...I tried with incorporating your changes but I seems like the similar behavior still on. The expected behaviour is until it get's navigated to the next screen / till successful login the indicator shouldn't show. And say lastlogin is indicating on Google and if the user click LinkedIn it should set to linkedin only when the user comes back again to login screen. Otherwise maybe you can try sharing a video if it works accordingly like mentioned.
Expected:

13.10.2024_12.02.42_REC.mp4

https://lex.page/auth
https://app.formbricks.com/auth/login

And a sample implementations can be find here
formbricks/formbricks#3376

@sahiladit
Copy link
Author

Hi @sahiladit, It's great to hear on your first contribution, many more to go! One small thing...I tried with incorporating your but I seems like the similar behavior still on. The expected behaviour is until it get's navigated to the next screen / till successful login the indicator shouldn't show. And say lastlogin is indicating on Google and if the user click LinkedIn it should set to linkedin only when the user comes back again to login screen. Otherwise maybe you can try sharing a video if it works accordingly like mentioned. Expected:

13.10.2024_12.02.42_REC.mp4
https://lex.page/auth https://app.formbricks.com/auth/login

And a sample implementations can be find here formbricks/formbricks#3376

I think i have resolved this ,try to run my code ,it's not merged yet,and if you still find it let me know.

@JeevaRamanathan
Copy link
Contributor

I tried with incorporating your
*changes

Hi @sahiladit, I’m aware the code hasn’t been merged yet. Before posting my previous comment, I incorporated your changes into the code (tried with your code), but the issue still remains. Would you mind checking it :-)

@sahiladit
Copy link
Author

I tried with incorporating your
*changes

Hi @sahiladit, I’m aware the code hasn’t been merged yet. Before posting my previous comment, I incorporated your changes into the code (tried with your code), but the issue still remains. Would you mind checking it :-)

Okay,thanks for the check, i'll try to resolve it.

@mfts
Copy link
Owner

mfts commented Oct 14, 2024

@sahiladit any update on the last changes?

@sahiladit
Copy link
Author

@sahiladit any update on the last changes?

Making some changes, will open a pr today.

@mfts
Copy link
Owner

mfts commented Oct 14, 2024

@sahiladit just add to this PR :)

@sahiladit
Copy link
Author

how can i change the code in existing pr?

@JeevaRamanathan
Copy link
Contributor

JeevaRamanathan commented Oct 15, 2024 •
edited
Loading

You can make changes in the same branch in your code(sahiladit:fix/last-used-indicator) and push it.

@sahiladit
Copy link
Author

i have made the changes,should i recheck it here?

@vercel Vercel
Copy link

vercel bot commented Oct 19, 2024 •
edited
Loading

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
papermark ❌ Failed (Inspect) Oct 19, 2024 9:32am

mfts
mfts requested changes Oct 19, 2024
View reviewed changes
Copy link
Owner

@mfts mfts left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sahiladit there's a build error.

TypeError: Cannot destructure property 'data' of '(0 , N.useSession)(...)' as it is undefined.
    at Z (/vercel/path0/.next/server/app/(auth)/login/page.js:1:9627)
    at nj (/vercel/path0/node_modules/next/dist/compiled/next-server/app-page.runtime.prod.js:12:46251)
    at nM (/vercel/path0/node_modules/next/dist/compiled/next-server/app-page.runtime.prod.js:12:47571)
    at nN (/vercel/path0/node_modules/next/dist/compiled/next-server/app-page.runtime.prod.js:12:64546)
    at nI (/vercel/path0/node_modules/next/dist/compiled/next-server/app-page.runtime.prod.js:12:47010)
    at nM (/vercel/path0/node_modules/next/dist/compiled/next-server/app-page.runtime.prod.js:12:47717)
    at nM (/vercel/path0/node_modules/next/dist/compiled/next-server/app-page.runtime.prod.js:12:61546)
    at nN (/vercel/path0/node_modules/next/dist/compiled/next-server/app-page.runtime.prod.js:12:64546)
    at nB (/vercel/path0/node_modules/next/dist/compiled/next-server/app-page.runtime.prod.js:12:67538)
    at nD (/vercel/path0/node_modules/next/dist/compiled/next-server/app-page.runtime.prod.js:12:66680) {
  digest: '600978353'
}
Error occurred prerendering page "/login". Read more: https://nextjs.org/docs/messages/prerender-error
TypeError: Cannot destructure property 'data' of '(0 , N.useSession)(...)' as it is undefined.
    at Z (/vercel/path0/.next/server/app/(auth)/login/page.js:1:9627)
    at nj (/vercel/path0/node_modules/next/dist/compiled/next-server/app-page.runtime.prod.js:12:46251)
    at nM (/vercel/path0/node_modules/next/dist/compiled/next-server/app-page.runtime.prod.js:12:47571)
    at nN (/vercel/path0/node_modules/next/dist/compiled/next-server/app-page.runtime.prod.js:12:64546)
    at nI (/vercel/path0/node_modules/next/dist/compiled/next-server/app-page.runtime.prod.js:12:47010)
    at nM (/vercel/path0/node_modules/next/dist/compiled/next-server/app-page.runtime.prod.js:12:47717)
    at nM (/vercel/path0/node_modules/next/dist/compiled/next-server/app-page.runtime.prod.js:12:61546)
    at nN (/vercel/path0/node_modules/next/dist/compiled/next-server/app-page.runtime.prod.js:12:64546)
    at nB (/vercel/path0/node_modules/next/dist/compiled/next-server/app-page.runtime.prod.js:12:67538)
    at nD (/vercel/path0/node_modules/next/dist/compiled/next-server/app-page.runtime.prod.js:12:66680)
   Generating static pages (8/35) 
   Generating static pages (17/35) 
   Generating static pages (26/35)

please resolvre that and try to npm run build it yourself locally first

@mfts mfts added revisit Currently not required but important in the future iteration-1 labels Oct 19, 2024
@mfts
Copy link
Owner

mfts commented Oct 22, 2024

@sahiladit please have a look at the last build error.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mfts mfts mfts requested changes

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
iteration-2 🕹️ oss.gg revisit Currently not required but important in the future 🕹️ 50 points
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@sahiladit @mfts @JeevaRamanathan