Skip to content

feature: Thankyou page linked to routes#128

Merged
danielhalasz merged 13 commits intodevfrom
thank-you
Jan 7, 2022
Merged

feature: Thankyou page linked to routes#128
danielhalasz merged 13 commits intodevfrom
thank-you

Conversation

@Mika215
Copy link
Collaborator

@Mika215 Mika215 commented Dec 19, 2021

!!! DO not MERGE This!

Hello team i have created the "Thank you component" and tried to integrate it to the main page.
However as i can see from the Figma design @WalterAlvar @Dabrytskaya its rather supposed to be a separate page(header,``thankyou component and footer).
But i have no idea how i can create a separate page with vue. I am guessing that we can have only one app.vue .
i would appreciate if you could give me some directions if not i am going to try to find a solution tomorrow.

@Mika215 Mika215 added the question Further information is requested label Dec 19, 2021
@Mika215 Mika215 added this to the must-have milestone Dec 19, 2021
@Mika215 Mika215 self-assigned this Dec 19, 2021
@Mika215 Mika215 linked an issue Dec 19, 2021 that may be closed by this pull request
@danielhalasz
Copy link
Collaborator

the answer to this is what I was just working on now 😄 please check solution in #129

@danielhalasz
Copy link
Collaborator

I have now created a route for this in the other PR..so the code can be copy+pasted

@Mika215 Mika215 changed the title Thank you Thankyou page linked to routes Dec 20, 2021
danielhalasz
danielhalasz previously approved these changes Dec 20, 2021
Copy link
Collaborator

@danielhalasz danielhalasz left a comment

Choose a reason for hiding this comment

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

works perfectly, great job!

@Mika215 Mika215 requested a review from danielhalasz December 20, 2021 20:13
@danielhalasz
Copy link
Collaborator

danielhalasz commented Dec 20, 2021

just not sure why your PR has so many changed files..have you tried merging the main branch before sending the PR?
ah sorry I guess just because it is to be merged to vuetest..which is something old..the one yesterday was vueRouter 😄 and was deleted after merge.. I usually try to clean up the repo and remove some unused old branches..now created a new branch for you PR v-thankyou..if it's ok

@danielhalasz danielhalasz changed the base branch from vuetest to v-thankyou December 20, 2021 20:24
Copy link
Collaborator

@katsmamina katsmamina left a comment

Choose a reason for hiding this comment

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

Hi @Mika215 ! Thank you for your contribution and for your speed, it's impressing! The page looks great, and I am happy to see that everything works with routes.

Just a few notes:

  • when I open the page in Safari, the button looks big and rather ugly 😄 (all is fine in Chrome), could you please check it and figure out? ...
  • in both browsers, the size of the logo, the font and the button is very big, so that the user needs to scroll to see everything (even with a smaller footer!) Or is it just me. Maybe it could be a bit more compact? 🤔

@katsmamina
Copy link
Collaborator

katsmamina commented Dec 20, 2021

Actually, when you hover the button in Safari, it just gets bigger ... and then bigger ... and then bigger! Until it takes the biggest part of the screen 😄

@danielhalasz danielhalasz changed the base branch from v-thankyou to dev December 21, 2021 13:15
@danielhalasz danielhalasz dismissed their stale review December 21, 2021 13:15

The base branch was changed.

@Mika215 Mika215 requested a review from katsmamina January 3, 2022 21:46
katsmamina
katsmamina previously approved these changes Jan 3, 2022
@Mika215
Copy link
Collaborator Author

Mika215 commented Jan 3, 2022

Hey Team,
could you take a look on the latest changes and give me your feedback?
i have made some slight modifications
margin and padding removed and no need of scrolling as before.
watch out it is showing me that there is some sort of conflict that needs to be resolved before merging .
I just saw this message after already pushing the changes.

Desktop 1240pxl
desktop-1240pxl

Tablet 768pxl
tablet768pxl

Mobile 320pxl
mobile-320pxl

@danielhalasz
Copy link
Collaborator

danielhalasz commented Jan 4, 2022

hi @Mika215 thank you it looks much better! Thanks for beginning work on the media queries too.
One observation is that the positioning of the elements could be fix, so all items are centered..now the logo & text are not aligned with the mainpage button..can you fix that?

@danielhalasz
Copy link
Collaborator

@Mika215 check if you agree with the centering..thank you

danielhalasz
danielhalasz previously approved these changes Jan 7, 2022
@danielhalasz danielhalasz changed the title Thankyou page linked to routes feature: Thankyou page linked to routes Jan 7, 2022
Copy link
Collaborator

@danielhalasz danielhalasz left a comment

Choose a reason for hiding this comment

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

LGTM

@danielhalasz danielhalasz merged commit b12c53c into dev Jan 7, 2022
@danielhalasz danielhalasz deleted the thank-you branch January 7, 2022 17:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

css html question Further information is requested

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Thank you page

3 participants