rehanvdm / serverless-website-analytics

A CDK construct that consists of a serverless backend, frontend and client side code to track website analytics
GNU General Public License v2.0
162 stars 11 forks source link

feat: Mobile friendly #86

Closed kc0bfv closed 2 months ago

kc0bfv commented 3 months ago

This gets the page closer to mobile friendly. With these changes, the site works significantly better on smaller screens. A remaining issue is the size of the map. Ref issue #15

Closes #15

rehanvdm commented 3 months ago

Hey thanks a lot for taking the time, I can already see that it's better than what it used to be. But if we ship this, then I would like a few tweaks to get it right the first time. See my comments and proposed changes in the rough sketch below.

image

For the map, I think the component will have to be split into two parts but the data must only be fetched once. Or maybe a better alternative is to just hide it on mobile and show the text table. It's difficult to find meaningful information when the map is so small on mobile, so I believe hiding it is the better alternative.

kc0bfv commented 3 months ago

Yeah, I like your tweaks! When viewing in test on my screens it didn't look quite that bad - and I didn't try the demo mode. What screen size approximately were you trying there? And, is there an easy way to turn on the demo banner when I'm just viewing the frontend with "npm run dev". Ooh

I'll try to look at this this weekend, and if there's not a good way to send in data then I'll hack something in so my work is better.

The first take was intended to be a light touch, and very size-flexible. I think these will require a heavier hand, maybe moving some parts of the templates a little and maybe duplicating some parts but having the duplicate display/not display based on screen width.

The map looks really cool - you're right it definitely needs to be two parts or just have the map disappear/hide. Fetching data only once is no problem, it should be possible to shift things around without changing how the underlying fetch/display code works.

On Mon, Apr 8, 2024, 01:47 Rehan van der Merwe @.***> wrote:

Hey thanks a lot for taking the time, I can already see that it's better than what it used to be. But if we ship this, then I would like a few tweaks to get it right the first time. See my comments and proposed changes in the rough sketch below.

image.png (view on web) https://github.com/rehanvdm/serverless-website-analytics/assets/22810856/9e615e8b-9d34-4e13-852a-654dd1c8a45e

For the map, I think the component will have to be split into two parts but the data must only be fetched once. Or maybe a better alternative is to just hide it on mobile and show the text table. It's difficult to find meaningful information when the map is so small on mobile, so I believe hiding it is the better alternative.

— Reply to this email directly, view it on GitHub https://github.com/rehanvdm/serverless-website-analytics/pull/86#issuecomment-2041981715, or unsubscribe https://github.com/notifications/unsubscribe-auth/AALGLJKC3SYUXOVWDSNWTNDY4I4Q3AVCNFSM6AAAAABF3CFHIOVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDANBRHE4DCNZRGU . You are receiving this because you authored the thread.Message ID: @.***>

kc0bfv commented 3 months ago

I found the contributing doc now with the backend instructions, btw.

rehanvdm commented 3 months ago

What screen size approximately were you trying there?

I was just using this feature of the browsers and selecting a few, not sure which size exactly, but this gives an idea image

And, is there an easy way to turn on the demo banner when I'm just viewing the frontend with "npm run dev".

If you get your backend running locally, small mention of it here then there is a flag that you set here in the test config along your other parameters.


Sorry, there are no frontend tests or easy ways to run and simulate large numbers as on the demo page. My idea was to tackle that problem if a lot of people struggle with getting it and tests running to revisit and improve it to make it easier. Let me know if you got npm run start-local-api-front command to work (not sure the watch ones actually watch). But that's how I develop on it, by starting the backend locally and then pointing the front end at it.

kc0bfv commented 3 months ago

I think it's in-line with the suggested view now! My commit 8ff3f1e has a long description attached with all the details about what changed. I kept the map at smaller sizes - I think the map is really cool... It does move down though.

I ended up writing a little service that just responded to API requests with canned responses copied out of the demo site - that was a simpler solution than figuring out how to do it right I think ;-)

localhost_5173_stats_page_sites= date=2024-04-07T05%253A00%253A00 000Z%252C2024-04-15T04%253A59%253A59 999Z(iPhone 12 Pro)

rehanvdm commented 3 months ago

This looks great! Thanks for all the effort.

I see a few regressions on the bigger view that I tried to capture in the image comparison below, let me know if something isn't clear or if you have questions/suggestions.

FinalChanges

ElementPlus has utilities to show/hide components depending on screen size https://element-plus.org/en-US/component/layout.html#utility-classes-for-hiding-elements, I tried to stick to using the framework as much as possible to this point. But I don't mind, I think it is fine like you have it, not too complicated.

Learned a few new things:

kc0bfv commented 3 months ago

Sounds good. The scrollbars on those elements - sorry about that. There was something weird happening in my browser off-and-on and it was kinda fixing it and not causing a problem. I think the interaction between plotly resizing and browser is just flaky when testing the way I am.

I am not going to make the hamburger, page/event, mobile view button bigger - because I'm not sure how much bigger to make it. Right now it's the same size as the refresh and gear buttons which... I think is nice.

The reason time on page column needed to be slightly wider to fit the large view is because the views column needed to be slightly wider as the screen narrowed even a little... I made time on page slightly wider to just get it done now... But I think the better solution might be to use just a regular HTML table, or to use just one grid view for the whole table instead of a grid view for the rows then a grid view for the columns for each row. I think both those solutions would allow the browser to determine the correct column widths on the fly, regardless of screen width, instead of statically determined at build time. I will mess around with that and see if it yields something more "responsive".

The built in elementplus utilities are interesting... The necessary CSS for that is not included in the site right now, we'd have to add element-plus/theme-chalk/display.css imported in src/main.ts, but there are still a few things that are marked with those utility classes, that'll disappear at small screen sizes, unnecessarily. The gear pop-out is marked that way, and because I copied it to make the page/events pop-out it's marked that way too. I'm removing that "hidden-sm-and-down" annotation there, then, so I can add this CSS in. Then I can mark things "hidden-xs-only" and "hidden-sm-and-up", but this moves the break size to 768px (no big deal).

I realized I don't have to change from grid to flex on screen width change, I can just change the number of columns :-) So I did that now. Simpler is better :-)

To make the site and date selectors be the original size on large screen, while still allowing them to flow to reasonable sizes/drop to another line, I added in another media size break at 900/899 px, where they're allowed to take up full width below that size, and they're allowed to wrap down a line.

Sorry to throw in two bugfixes in the middle of these commits, too, but I noticed JS errors in the console when I was working on this, and fixed a typo in TableData.vue (v-lese instead of v-else), and the way the tooltip content was calculated in there (had to convert the number to a string for output).

rehanvdm commented 2 months ago

Thanks! This looks great.

Thanks for picking up those typos, thought vue-tsc would pick up on that, seems not.

You mentioned that you still want to look at using a table view instead? Or is this ready to be merged? I am happy with the grid view for now. I can't remember why I changed to grid instead of table, I think it might have been because I needed to hide the site column when more than 1 site is selected or something, not sure.

rehanvdm commented 2 months ago

Pulling the trigger. Thanks for this contribution! 🍻

github-actions[bot] commented 2 months ago

:tada: This PR is included in version 1.10.0 :tada:

The release is available on:

Your semantic-release bot :package::rocket:

kc0bfv commented 2 months ago

Yay! Sorry about lack of response. I'll experiment with tables somewhere else and let you know if something nice comes out.

On Sat, May 4, 2024, 05:44 github-actions[bot] @.***> wrote:

🎉 This PR is included in version 1.10.0 🎉

The release is available on:

Your semantic-release https://github.com/semantic-release/semantic-release bot 📦🚀

— Reply to this email directly, view it on GitHub https://github.com/rehanvdm/serverless-website-analytics/pull/86#issuecomment-2094115989, or unsubscribe https://github.com/notifications/unsubscribe-auth/AALGLJN23V5IYSPQPK73W5TZAS3YPAVCNFSM6AAAAABF3CFHIOVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDAOJUGEYTKOJYHE . You are receiving this because you authored the thread.Message ID: @.***>