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.

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

feat(project): update edit and create forms #339

Merged
merged 8 commits into from
Jul 18, 2023

Conversation

naumovski-filip
Copy link
Contributor

@naumovski-filip naumovski-filip commented Jul 14, 2023

Description

Config: maqet3qh

https://www.figma.com/file/qLl8KxBOOr640h18ICx0H6/Web-app?type=design&node-id=2344-62912&mode=design

Steps completed:

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

  • Acceptance criteria met
  • Unit tests added
  • Docs updated (including config and env variables)
  • Translations added
  • UX tested
  • Browsers / platforms tested
  • Rebased & ready to merge without conflicts
  • Reviewed own code

@github-actions
Copy link

github-actions bot commented Jul 14, 2023

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

https://ottwebapp--pr339-feat-profiles-edit-c-fn2ntxm0.web.app

(expires Thu, 17 Aug 2023 11:01:50 GMT)

🔥 via Firebase Hosting GitHub Action 🌎

Sign: c198f8a3a199ba8747819f7f1e45cf602b777529

Comment on lines 95 to 96
{t('profile.greeting')}
{`${fullName && ','} ${fullName}`}
Copy link
Collaborator

Choose a reason for hiding this comment

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

It might be better to include the name placeholder in the translations because we can't guarantee that the sentence structure is the same for all languages.

Suggested change
{t('profile.greeting')}
{`${fullName && ','} ${fullName}`}
{fullName ? t('profile.greeting_with_name', { fullName }) : t('profile.greeting')}

translations:

{
  "profile": {
    "greeting_with_name": "Greetings, {{fullName}}!"
  }
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@dbudzins
Copy link
Contributor

@naumovski-filip Just a logistical question, but why are we opening PR's into feature branches? Is this code that needs to be reviewed or should it wait until the entire feature is ready? Or better yet can it be added incrementally into the develop branch?

@naumovski-filip
Copy link
Contributor Author

@naumovski-filip Just a logistical question, but why are we opening PR's into feature branches? Is this code that needs to be reviewed or should it wait until the entire feature is ready? Or better yet can it be added incrementally into the develop branch?

It should be reviewed yes, the idea is to merge feat/profiles into the hackweek branch (when it's ready) which already has an open PR to keep proper history. feat/profiles is created from the hackweek branch and a lot of refactoring was necessary because that branch was outdated so that's why we decided with this approach.

@dbudzins
Copy link
Contributor

@naumovski-filip Just a logistical question, but why are we opening PR's into feature branches? Is this code that needs to be reviewed or should it wait until the entire feature is ready? Or better yet can it be added incrementally into the develop branch?

It should be reviewed yes, the idea is to merge feat/profiles into the hackweek branch (when it's ready) which already has an open PR to keep proper history. feat/profiles is created from the hackweek branch and a lot of refactoring was necessary because that branch was outdated so that's why we decided with this approach.

I'm not sure exactly how that is going to work though. The code in the hackweek branch and the feat/profiles branch has not been fully reviewed, so reviewing the code going into those branches doesn't ensure that either one will be ready to merge to develop without doing another full review, which I expect is going to have a lot of changes at this point. Am I missing something? If not, let's discuss this approach before you go any further with it, because I'm not sure it's the best way for us to proceed.

@naumovski-filip naumovski-filip merged commit 7a7a209 into feat/profiles Jul 18, 2023
4 of 6 checks passed
@naumovski-filip naumovski-filip deleted the feat/profiles-edit-create-form branch July 18, 2023 15:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants