-
Notifications
You must be signed in to change notification settings - Fork 3
GH-75: Making the application mobile responsive #79
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
base: main
Are you sure you want to change the base?
Conversation
…g landscape detection
Summary of ChangesHello @yasandu0505, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the application's mobile responsiveness, ensuring a consistent and user-friendly experience across a wide range of devices. It introduces a novel approach to managing complex UI elements on smaller screens by enforcing a landscape orientation when necessary, and systematically refines the layout and presentation of key components throughout the application. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request introduces significant improvements to the application's mobile responsiveness. The changes refactor UI components to adapt to different screen sizes, including a new custom hook to enforce landscape orientation for complex UI. My review identified a critical issue in the new landscapeRequired hook that would break server-side rendering, a typo in a CSS class name, and a minor maintainability issue with a hardcoded breakpoint. Overall, the changes are well-aligned with the goal of improving the mobile user experience.
|
@yasandu0505 when you need to link the corresponding issue, you need to say
|
No,it links the issue to the PR. That's how even zaeema did yesterday , I haven't just hard coded the issue number . It has been selected and picked , so it connects to the issue |
@yasandu0505 you have to say closes #75 not just adding the issue id as #75. |
Okay, My bad , Tank you! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in the ministry card grid. there is a row containing search bar, filter dropdown and view toggle. That section needs to be corrected for proper responsiveness for smaller screens
|
These icons are a bit fancy. maybe something like this Icon Open for suggesions
|
ChanukaUOJ
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added Initial Comments
src/components/landscapeRequired.jsx
Outdated
| export default function LandscapeRequired({ | ||
| smallMaxWidth = 768, | ||
| children, | ||
| message = "Rotate your device to landscape 📱↔️" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- This icon should be changed as i mentioned above
- "Rotate your device" is enough?
src/components/landscapeRequired.jsx
Outdated
| const overlayStyle = { | ||
| position: "fixed", | ||
| inset: 0, | ||
| display: "flex", | ||
| justifyContent: "center", | ||
| alignItems: "center", | ||
| background: "rgba(0,0,0,0.9)", | ||
| color: "#fff", | ||
| fontSize: "20px", | ||
| textAlign: "center", | ||
| zIndex: 9999, | ||
| padding: "16px" | ||
| }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Better to use tailwind classes for styling
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I kept it like this , because it is easier to customize the overlay, let's discuss
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Something I already mentioned above to change the styling of this in the mobile view
| const navigate = useNavigate(); | ||
| const location = useLocation(); | ||
| const { tab } = useParams(); | ||
| const MOBILE_BREAKPOINT = 768; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it better to take this from a config so no need to initialize a new variable whenever we needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
but the mobile breakpoint never changes (That is why i kept it as a constant). so it does not matter , what do you think ?
Is it better to take this from a config so no need to initialize a new variable whenever we needed.
These should be configured by the way you create the hover effects. Generally, when we add a hover effect , on the desktop it appears when we hover , and on mobile it appears on the when we click on it. example -> go to live project section. |
|
@yasandu0505 Test this in local with the new changes. It seems like some crashes are there |
















-Refactored the application to behave user friendly in different screen sizes. New logics for rendering components have implemented.
-Changed some ui components to display them on a small screen in an efficient way.
-And also added a new custom hook for disable the viewing of complex ui components on the portrait mode of the screen (complex ui components are required to have at least a landscape orientation). the hook is reusable with any component.
This PR closes #75
Updated Later -> This PR handles the responsiveness of the Organization panel only.