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

Veue 211 - Improve "Videos#index" display #123

Merged
merged 1 commit into from
Nov 12, 2020
Merged

Veue 211 - Improve "Videos#index" display #123

merged 1 commit into from
Nov 12, 2020

Conversation

Seraphyne
Copy link
Contributor

Branch VEUE-211:

  • Made changes on _layout.scss to show the h2 "Recent Video" responsive on desktop, mobile: portrait and landscape (according to breakpoints.scss)
  • Commented out the // min-width: 0; on _global.scss

@HamptonMakes
Copy link
Contributor

Task linked: VEUE-211 Improve "Videos#index" display

@HamptonMakes
Copy link
Contributor

@Seraphyne can you attach some screenshots? If you take a screenshot, Github lets you drop the image right into the text field and will automatically upload it! MAGIC.

@Seraphyne
Copy link
Contributor Author

Seraphyne commented Nov 10, 2020

Here are the screenshots:

On Chrome:

- Large

chrome-desktop-large

- Small

chrome-desktop-small

On Firefox:

- Large

firefox-desktop-large

- Small

firefox-desktop-small

On IphoneXR:

- Landscape mode

iphoneXR-landscape

- Portrait mode

iphoneXR-portrait

@@ -5,7 +5,7 @@ body,
* {
font-family: font.$nunito;
font-weight: font.$normal-weight;
min-width: 0;
// min-width: 0;
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm pretttty sure this is going to break the video watch page!

desktop.ini Outdated
[ViewState]
Mode=
Vid=
FolderType=StorageProviderGeneric
Copy link
Contributor

Choose a reason for hiding this comment

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

@Seraphyne Do you know what this file is?

Copy link
Contributor Author

@Seraphyne Seraphyne Nov 10, 2020

Choose a reason for hiding this comment

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

I saw it ran changes on it, but I didn't personally changed it.

@@ -28,7 +28,7 @@ $veue-lilac: #6460f1;
$neutral-dark: #40424f;
$neutral-regular: #9595a5;
$neutral-middle: #c1c1cc;
$neutral-soft: #eaeaf00;
$neutral-soft: #eaeaf0;
Copy link
Contributor

Choose a reason for hiding this comment

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

Was this intentional?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nope! I dunno what happened there..

flex-wrap: wrap;
margin: 3rem;
}
}
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 this would be better column, maybe wrap the h2 in a div and let it just push the contents down?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will restart the changes right now..

@Seraphyne
Copy link
Contributor Author

VEUE-211 - Changes made:

  • on global.scss I put back the min-width

  • put the desktop.ini into gitignore

  • put the color back at color.scss (the zero it was missing)

  • on layout.scss, I used flex-direction:column and used some margin-left to make the videos and the h2 "Recent Videos" to look aligned.

Here are the pictures of the final result:

full screen
mobile

@Seraphyne
Copy link
Contributor Author

Oh, and since I had to install the webpacker again, it changed the version to "@rails/webpacker": "5.2.1", so the other files that was changed, has to do with my re-installation of the webpacker.

@HamptonMakes
Copy link
Contributor

Needs rspec to pass!

@Seraphyne
Copy link
Contributor Author

Changes that I have done:

  • on layout file: removed the media queries since there were extra and the site looked the same on the screenshots. (see bellow)

Changes that Rubocop did:

  • All the changes you will see on the bin files. Those were made when I ran the command: rubocop -A

I see on the comment saying that "This branch has conflicts that must be resolved" is because Windows weren't starting my server. The error I was having was: ERROR: worker mode not supported. For that reason, I saw the solution about it in here. So I went and commented out the line # workers ENV.fetch("WEB_CONCURRENCY") { 4 } @config/puma.rb. If this line is important, I don't mind commenting it out and in every time I ran the server and NOT adding to my commit. Not a problem!

Please, let me know if all the differences are corrected or if I need to tweak something else.

Here are the pictures:
chrome
Iphone

@HamptonMakes HamptonMakes force-pushed the VEUE-211 branch 2 times, most recently from 7ac7710 to a24adf4 Compare November 12, 2020 19:34
Copy link
Contributor

@HamptonMakes HamptonMakes left a comment

Choose a reason for hiding this comment

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

LGTM (this is renata's code)

@HamptonMakes HamptonMakes merged commit 74b1f33 into main Nov 12, 2020
@HamptonMakes HamptonMakes deleted the VEUE-211 branch November 12, 2020 20:02
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