Skip to content
This repository has been archived by the owner on Mar 23, 2023. It is now read-only.

feat: revamp the introductory screens and allow choosing language #1074

Merged
merged 13 commits into from Feb 15, 2019

Conversation

j-a-m-l
Copy link
Contributor

@j-a-m-l j-a-m-l commented Feb 12, 2019

Proposed changes

This PR revamps the introductory screens and add the possibility to choose language at the beginning.

Types of changes

  • New feature (non-breaking change which adds functionality)
  • Refactoring (improve a current implementation without adding a new feature or fixing a bug)

Checklist

  • I have read the CONTRIBUTING documentation
  • Lint and unit tests pass locally with my changes

@j-a-m-l j-a-m-l added the Type: Feature The issue is a request for new functionality. label Feb 12, 2019
@ghost ghost added the Complexity: Undetermined Needs specialized, in-depth review. label Feb 12, 2019
@codecov-io
Copy link

codecov-io commented Feb 12, 2019

Codecov Report

Merging #1074 into develop will decrease coverage by 0.13%.
The diff coverage is 0%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #1074      +/-   ##
===========================================
- Coverage    39.62%   39.48%   -0.14%     
===========================================
  Files          205      204       -1     
  Lines         5164     5164              
  Branches      1013     1014       +1     
===========================================
- Hits          2046     2039       -7     
- Misses        2998     3003       +5     
- Partials       120      122       +2
Impacted Files Coverage Δ
src/renderer/App.vue 0% <0%> (ø) ⬆️
...enderer/components/App/AppIntro/AppIntroScreen.vue 0% <0%> (ø)
src/renderer/components/App/AppIntro/index.js 0% <0%> (ø)
src/renderer/components/App/index.js 0% <0%> (ø) ⬆️
src/renderer/components/App/AppIntro/AppIntro.vue 0% <0%> (ø)
src/renderer/store/modules/peer.js 70.56% <0%> (-1.87%) ⬇️
src/renderer/services/client.js 66.22% <0%> (-1.01%) ⬇️
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a7fddee...575461c. Read the comment docs.

@j-a-m-l j-a-m-l changed the title [WIP] feat: revamp the introductory screens and allow choosing language feat: revamp the introductory screens and allow choosing language Feb 13, 2019
@j-a-m-l j-a-m-l added the Status: Needs Review The issue or pull request needs a review by a developer of the team. label Feb 13, 2019
@@ -0,0 +1,142 @@
<template>
<div class="flex flex-row w-full h-full">
<div class="AppIntroScreen__container__left flex-1 animated fadeIn rounded-lg">
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it should follow the same responsiveness as the other screens:

Suggested change
<div class="AppIntroScreen__container__left flex-1 animated fadeIn rounded-lg">
<div class="AppIntroScreen__container__left flex-1 animated fadeIn rounded-lg hidden lg:block">

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, removing the left part makes it better for smaller screens

v-if="showFooter"
name="buttons"
>
<div class="flex flex-row w-full justify-between px-16 pb-16 sm:px-10 sm:pb-10">
Copy link
Contributor

Choose a reason for hiding this comment

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

The initial style should be switched, and replicated to the parent elements.

Suggested change
<div class="flex flex-row w-full justify-between px-16 pb-16 sm:px-10 sm:pb-10">
<div class="flex flex-row w-full justify-between px-10 pb-10 md:px-16 md:pb-16">

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've removed this; since now the left part (the image) is removed, this trick isn't necessary

<MenuDropdown
:items="languages"
:value="language"
:position="['-50%', '0%']"
Copy link
Contributor

Choose a reason for hiding this comment

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

This positioning is awkward because it is misaligned to the input. I think you making the same adjustments made to the ProfileEdition will look better.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, I didn't realize how awkward was. I've also improved the style of the items.

src/renderer/components/App/AppIntro/AppIntro.vue Outdated Show resolved Hide resolved
@ghost ghost added the Status: Needs Changes The pull request needs additional changes before it can be merged. label Feb 13, 2019
@ghost
Copy link

ghost commented Feb 13, 2019

@j-a-m-l Your pull request needs some changes. Please wait for a comment from one of our developers for more information.

@j-a-m-l j-a-m-l removed the Status: Needs Changes The pull request needs additional changes before it can be merged. label Feb 14, 2019
@j-a-m-l
Copy link
Contributor Author

j-a-m-l commented Feb 14, 2019

@luciorubeens I've followed your suggestions and included some additional changes

@j-a-m-l j-a-m-l removed the Complexity: Undetermined Needs specialized, in-depth review. label Feb 14, 2019
@j-a-m-l j-a-m-l merged commit 60f7277 into develop Feb 15, 2019
@ghost ghost deleted the new-intro-screens branch February 15, 2019 09:35
@ghost ghost removed the Status: Needs Review The issue or pull request needs a review by a developer of the team. label Feb 15, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Type: Feature The issue is a request for new functionality.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants