Skip to content

task1_yael - #1

Open
yael-dangor wants to merge 2 commits into
mainfrom
task1_yael
Open

task1_yael#1
yael-dangor wants to merge 2 commits into
mainfrom
task1_yael

Conversation

@yael-dangor

Copy link
Copy Markdown

No description provided.

@omrirz omrirz left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

General comments:

  1. please change "asstst" directory to "assets".
  2. please create a subdirectory in assets for icons and a subdirectory for images. we don't want a big mess of files.
  3. please change file names to a consistent pattern. You now have "components/service_card.tsx" (which is snake case) and "components/contactForm.tsx" (which is camel case). change all of the files to be kebab case e.g. "service-card.tsx", "contact-form.tsx", etc.
  4. You deleted the pages/index.tsx which had valuable components and was holding the home page structure, you should bring it back and use it instead of the changes you did to _app.tsx. See my comment about it below.

Comment thread components/whatWeDo.tsx Outdated
Comment thread pages/_app.tsx Outdated
<Layout pageProps={pageProps}>
<Component {...pageProps} />
</Layout>
<>

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

this _app.tsx file is a file for all the pages in the app so now we can't create more pages since you hardcoded those components for the entire app.
You should revert this file and use the pages/index.tsx for the homepage.

Comment thread pages/api/data.js Outdated
@@ -0,0 +1,16 @@
// an simple endpoint for getting current list

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Do we need this file?

Comment thread pages/index.js Outdated
},
)

const addMutation = useMutation((value) => fetch(`/api/data?add=${value}`), {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Why we have todos here?

Comment thread pages/index.js Outdated
@yael-dangor
yael-dangor requested a review from omrirz February 17, 2022 09:59
@yael-dangor

Copy link
Copy Markdown
Author

I have a server error:
Server Error
Error: GraphQL Error (Code: 401): {"response":{"data":[{"id":"5413bc","type":"api_error","attributes":{"code":"INVALID_AUTHORIZATION_HEADER","details":{}}}],"status":401,"headers":{}},"request":{"query":"\nquery Homepage {\n homepage(locale: heb) {\n title\n description\n getToKnowUsButton\n aboutHeading\n aboutTitle\n aboutUsText\n aboutUsButton\n whatWeDoHeading\n whatWeDoTitle\n whatWeDoCards {\n title\n text\n imagePath\n }\n contactUsHeading\n contactUsTitle\n contactUsText\n }\n \ncommon {\n contactUsLinks {\n text\n linkType\n imagePath\n }\n contactUsFormFirstName\n contactUsFormLastName\n contactUsFormEmail\n contactUsFormPhone\n contactUsFormMessage\n contactUsFormSendButton\n}\n\n}\n"}}
I tried to solve with a lot of options,
if you can help

@omrirz

omrirz commented Feb 27, 2022

Copy link
Copy Markdown
Contributor

@yael-dangor you sure you have all the necessary environment variables like in .env.example?

@yael-dangor

Copy link
Copy Markdown
Author

Do i need a token?

@omrirz omrirz left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

  1. הלינקים לא עובדים

Screen Shot 2022-04-25 at 21 25 52

  1. התמונות לא מוצגות טוב

Screen Shot 2022-04-25 at 21 28 34

  1. אייקונים לא מופיעים

Screen Shot 2022-04-25 at 21 28 50

  1. אין ולידציה על הטופס

Screen Shot 2022-04-25 at 21 31 53

  1. כפתורי החלפת שפה לא מחליפים שפה.
  2. הקישורים בראש העמוד ״אודות המרפאה״ ו- ״צור קשר״ קופצים לסקשן הרלוונטי במקום לגלול אליו.

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.

2 participants