699 Update responsive design up to 200% zoom

Kenny4297 commented 10 months ago

Description

I have implemented media queries across all CSS files pertaining to the Header. These modifications ensure proper display and functionality of the header at up to 200% zoom.

Screenshots

Here's how the header looks at 200% zoom: 200Zoom

Notes

While this project's structure—with multiple components and corresponding CSS files—works well with JavaScript logic implementation, it presents challenges in styling, particularly for the header.

Concerns

In my personal opinion, I would be worried about the scalability of this project when it comes to responsive design. Although it is possible to make this application responsive, the fact that multiple CSS files are used (and scattered) throughout the application with regards to one section of the application (the header, for example) will make responsive design difficult.

Thanks for letting me contribute!!

pmarsh-scottlogic commented 10 months ago

Hiya @Kenny4297 - that was fast work!

Regarding the concerns - yes, I agree it is hard to make the site responsive! Have you come across any project structures that better enable responsive design? Any examples? We're happy to learn!

Make sure you run the formatter on the frontend - the workflow is complaining. (cd frontend then npm run format)

And finally a heads up and apology - all the maintainers work on this as part of our day job, and it is coming up to Christmas hols, so I'm afraid we'll only be able to give this PR some attention after the new year. So take care until then :)

p.s. for my own interest, how did you come across the project?

Kenny4297 commented 10 months ago

Hi Scott:

Regarding project structure: From my experiences, I would put all the header CSS in one file. This way it makes things much easier.

Formatter: I can run the formatter and give you another PR soon.

How I found this project: I used the following site: https://goodfirstissues.com/index.html It's surprising how difficult many of the first issues are, so to keep things simple (especially with my day job as a lead engineer) I use this site to quickly find any applications that need 'basic' assistance.

Additional Notes: As an AI engineer myself, I am quite keen on the type of code that AI puts out. In my personal opinion, some of the code is over complicated and can be refactored. The much of the code in the header, in my opinion, is overkill, and thus it makes it hard to edit and adjust.

Thanks again for letting me work on this, it was a challenge! Also, connect with me on LinkedIn!

https://www.linkedin.com/in/kedgard-cordero/

Happy Holidays, Kedgard

On Fri, Dec 22, 2023 at 11:54 AM Peter Marsh @.***> wrote:

Hiya @Kenny4297 https://github.com/Kenny4297 - that was fast work!

Regarding the concerns - yes, I agree it is hard to make the site responsive! Have you come across any project structures that better enable responsive design? Any examples? We're happy to learn!

Make sure you run the formatter on the frontend - the workflow is complaining. (cd frontend then npm run format)

And finally a heads up and apology - all the maintainers work on this as part of our day job, and it is coming up to Christmas hols, so I'm afraid we'll only be able to give this PR some attention after the new year. So take care until then :)

p.s. for my own interest, how did you come across the project?

— Reply to this email directly, view it on GitHub https://github.com/ScottLogic/prompt-injection/pull/727#issuecomment-1867940241, or unsubscribe https://github.com/notifications/unsubscribe-auth/AXE6WKRMEXYMKBORDML43V3YKXCL7AVCNFSM6AAAAABBABWS3CVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQNRXHE2DAMRUGE . You are receiving this because you were mentioned.Message ID: @.***>

Kenny4297 commented 10 months ago

I really like the changes that have been implemented. I'd like to provide some additional insights:

1) Scaling of Font Sizes with Zoom Levels

Observation:

"Since all the font sizes are defined in rem, it means that when you adjust the zoom level, all the fonts will scale exactly according to that zoom level."

Comment:

This statement may not be entirely accurate. rem units are relative to the root element's font size, not directly responsive to changes in the window size like percentages or viewport units (vw, vh).
To make font sizes responsive to window size using rem, you would typically adjust the root element's font size with media queries.

Suggestion:

What you could do is set the root font sizes through media queries. This way, font sizes defined in rem within those roots would change automatically in response to window size adjustments.

2) Shortening '0rem' to '0'

Proposal:

Add all these modifications to your formatter!. This could streamline the process and ensure consistency across the codebase.

Note:

Due to my current workload, I might not be able to revisit this for the next few months. I'm grateful for the opportunity to contribute and look forward to seeing how this project progresses.

Thanks for letting me work on this!

Best regards, Kedgard

gsproston-scottlogic commented 10 months ago

Hi Kedgard,

Thanks for your work on this, and your patience while most of us were away over the winter break. If you're unable to work on this for the next few months, I'll close this PR if that's all right. Again, thank you for your work and your insight regarding the project structure and adding formatting rules for CSS, we'll definitely be looking into those!

Kind regards, George

chriswilty commented 10 months ago

@Kenny4297 FWIW We've not really looked into responsive design yet, and we don't even have issues in our backlog for it, at the moment, as there are UX challenges around using this application on smaller devices. That being said, as you suggest we would likely want to use a limited set of media queries to set some values application-wide, such as base font size and maybe a spacing/padding strategy.

Zoom is of course tricky, because it can affect which media query is active. We could maybe use resolution in dppx to distinguish if we are zoomed in or just on a smaller device. As @pmarsh-scottlogic points out, we don't really want an increase in zoom to decrease the font size.

pmarsh-scottlogic commented 9 months ago

Hi @Kenny4297

We're closing the PR for now since you won't have time to work on it, and we'd like to discuss within the team and be sure on what the outcome of the issue should be before work continues. Apologies that the requirements weren't crystal clear. Also in hindsight, it might be a bit in-depth for a "good first issue".

That being said, it was good to have somefresh insight to inform our choices going forward. In particular, we're already working to add some css linting to deal with the 0rem problem :)

If you want to continue work on the issue when you have more free time, then please leave us a comment and we'll happily discuss!

Happy new year

Kenny4297 commented 9 months ago

Thanks again for letting me work on it! I'll reach back out if I get some time for either this issue or others!

ScottLogic / prompt-injection