jwplayer / ott-web-app

Reference implementation for JWP-powered apps
Apache License 2.0
70 stars 52 forks source link

Chore: Detecting password presence for users registered via social providers #392

Closed borkopetrovicc closed 9 months ago

borkopetrovicc commented 10 months ago

Description

Notion card: Detecting password presence for users registered via social providers

This PR solves # .

Steps completed:

According to our definition of done, I have completed the following steps:

github-actions[bot] commented 10 months ago

Visit the preview URL for this PR (updated for commit b3aa739):

https://ottwebapp--pr392-social-providers-pas-9bbd4dgc.web.app

(expires Fri, 15 Dec 2023 16:21:59 GMT)

🔥 via Firebase Hosting GitHub Action 🌎

Sign: c198f8a3a199ba8747819f7f1e45cf602b777529

borkopetrovicc commented 10 months ago

LGTM!

  • Have you run yarn i18next to extract all translations?
  • Perhaps you can rebase/squash the commits to a single commit to make it cleaner. There are some faulty commit messages as well.

I have run yarn i18next. Good point for the commits. I will be more carefully for others pr-s.

borkopetrovicc commented 9 months ago

@ChristiaanScheermeijer I initially believed that you had already approved and commented 'LGTM'. Could we address these recent comments separately in another PR or ticket for improvements?

kiremitrov123 commented 9 months ago

@ChristiaanScheermeijer I initially believed that you had already approved and commented 'LGTM'. Could we address these recent comments separately in another PR or ticket for improvements?

@borkopetrovicc the suggestions make sense to me and do not look like a lot of effort, so we better address them in this PR

ChristiaanScheermeijer commented 9 months ago

@ChristiaanScheermeijer I initially believed that you had already approved and commented 'LGTM'. Could we address these recent comments separately in another PR or ticket for improvements?

Yes, sorry for that. I indeed approved after a quick scan, but after a second review (and some new changes), I have found new improvement points.

The suggested changes will not affect the outcome but will improve the code quality and testability of components. The changes shouldn't take long, so I agree with @kiremitrov123 to include them in this PR. But that's up to you :-)

naumovski-filip commented 9 months ago

@ChristiaanScheermeijer @AntonLantukh Because @borkopetrovicc is on sick leave I've taken a look at the most recent comments and tried to fix them. Regarding the failing E2E tests, there are issues on Cleeng's side.