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

make event list more compact on mobile #82

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

marcelkalveram
Copy link

This is a relatively simple, initial approach to making the mobile site more compact.

I didn't let the image float next to the title because I think it gives the title too little space. Especially with longer titles, they'll wrap to the next line, making it look more awkward.

Happy to hear your suggestions to improve this further.

Here are two screenshots for preview:

iPhone X: https://www.dropbox.com/s/tub1pb849kkzz12/Screenshot%202019-10-08%2022.57.51.png?dl=0

Pixel XL: https://www.dropbox.com/s/9pdtgor68vlauyr/Screenshot%202019-10-08%2022.58.04.png?dl=0

Copy link
Member

@hell03610 hell03610 left a comment

Choose a reason for hiding this comment

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

I'm more inclined for a list for small screens with a design more like the one in the mobile app (maintaining the general styles of the web for links, separators, etc), but that will probably require also some changes in the html strucutre, so that the medium/large screens dont get affected.

But all in all, your proposal is a better design than the one we currently have, so if you want to go with it, go ahead after solving the code change suggested. Thanks a lot!!

@@ -14,9 +14,12 @@
.event-summary {
position: relative;
justify-self: left;
margin-bottom: $space-large;
width: 100%;
margin-bottom: $space-large/2;
Copy link
Member

Choose a reason for hiding this comment

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

instead of dividing, try it with @space-medium -

@@ -67,6 +71,9 @@
height: 2px;
color: black;
background-color: black;
margin: $space-large auto $space-larger auto;
margin: $space-large/2 auto $space-larger/2 auto;
Copy link
Member

Choose a reason for hiding this comment

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

same here, try with $space-medium in both or medium - large instead of the divisions, thx!

@marcelkalveram
Copy link
Author

Thanks for the feedback @hell03610 . I'll fix the two comments.

Agree that it would be good to be consistent, but playing a bit with the layout, I realised that I actually like the idea of having the avatar sitting on top of the title.

I'll give that another try and get back to you here.

@marcelkalveram
Copy link
Author

Would you rather prefer something like this?

I personally don't dislike it, but I find it a tiny bit too squeezed, given the horizontal margin which doesn't exist in this way in the app.

Screenshot 2019-10-10 22 52 26

@hell03610
Copy link
Member

I like it! I would try with smaller avatars and slightly less margin around the separator line, so there is more space for the title - but I like it!!

@marcelkalveram
Copy link
Author

marcelkalveram commented Oct 15, 2019

I've pulled in the latest master branch and applied the changes (smaller avatar, less margin). I'm not sure I understood the idea about reducing the margin around the separator line. I guess you're referring to the <hr> and vertical margins around it?

What do we do with the new badge? (hoy, esta semana). I think it could go above the title, but as a smaller text, maybe?

Screen Shot 2019-10-15 at 14 15 00

@hell03610
Copy link
Member

Thanks @marcelkalveram! Yes, the today/this week label was a pr this week. I would simply hide it in mobile for now and go ahead with this pull request. I'm really looking to merging it, the design looks very nice!

@marcelkalveram
Copy link
Author

@hell03610 done!

Copy link
Member

@hell03610 hell03610 left a comment

Choose a reason for hiding this comment

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

Interface looks very nice, but the current codebase does not use floats and I'd like to keep it that way, could you rephrase it to use flex or grid instead? thx!

}
}

.reset-link {
margin-bottom: $space-small;
background-image: none;
float: left;
Copy link
Member

Choose a reason for hiding this comment

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

could you use flex instead of float?

Copy link
Author

Choose a reason for hiding this comment

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

I'll work on that this week. I think because of the nesting, float was the easier solution, but I'll see if we can use flexbox instead.

Copy link
Author

Choose a reason for hiding this comment

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

Sorry, I can't get this done easily because I'd need to change the HTML structure. Right now, I don't have lots of time, so I don't think it'll be possible in the next 1-2 weeks.

@@ -77,13 +98,22 @@
text-align: left;
color: $text-color-secondary;
font-size: 0.9rem;
margin-left: $thumbnailSizeMobile;
Copy link
Member

Choose a reason for hiding this comment

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

I hope that with flex/grid instead of float this margin-left is not needed. But if it is truly needed, could you move it to https://github.com/VLCTechHub/VLCTechHub-site/blob/master/data/assets/css/abstract/_variables.scss in kebab-case?

Copy link
Author

Choose a reason for hiding this comment

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

I'll take a look as soon as the float issue is resolved

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