Issue855. Make Main Landing Page Responsive and Add Hamburger Menu for Mobile View . #949

AradhyaDixit18 · 2024-07-01T18:15:45Z

This pull request enhances the responsiveness of the main landing page by introducing a hamburger menu for mobile devices. The following changes have been implemented:

Added Hamburger Menu:
Introduced a hamburger menu icon that appears on screens with a width of 768px or less.
The hamburger menu provides easy navigation on mobile devices by collapsing the side navigation into a drawer.

CSS Media Queries:
Utilized CSS media queries to ensure the hamburger menu icon is only displayed on mobile devices.
Adjusted styles for various elements to improve their layout and presentation on smaller screens.

Conditional Rendering in React:
Updated the React component to conditionally render the hamburger menu based on the screen size.
Added state management to handle the opening and closing of the hamburger menu drawer.

Styling Adjustments:
Ensured that the header remains sticky and spans the full width of the page.
Centered the main content on the page and provided adequate padding for a better user experience on all devices.

Now it looks like this :

Update main from dev finos#3

netlify · 2024-07-01T18:16:01Z

✅ Deploy Preview for dapper-ganache-45a60b ready!

Name	Link
🔨 Latest commit	`cdd7a62`
🔍 Latest deploy log	https://app.netlify.com/sites/dapper-ganache-45a60b/deploys/6692850a77b9070008b8539c
😎 Deploy Preview	https://deploy-preview-949--dapper-ganache-45a60b.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Sweetdevil144 · 2024-07-02T02:12:11Z

code/src/data/themes

@@ -1,27 +1,6 @@
-{"k":"0000000080","o":"0000002870","v":"001"}


Why are large deletions in pre existing themes?

@AradhyaDixit18 I'm guessing that you probably committed all changed files in the repo and didn't mean to commit this file? The code/src/data/themes file should only be committed if we are changing one of the default design systems that we ship with the project. Which doesn't seem to be the case with this PR.

code/src/ui/src/pages/WelcomePage.css

Sweetdevil144 · 2024-07-02T02:15:14Z

code/src/ui/src/pages/atoms/ColorPaletteAtom.css

+
+

Can you review these extra lines. Just good code-quality

code/src/ui/src/pages/atoms/ColorPaletteAtom.tsx

Sweetdevil144 · 2024-07-02T02:19:09Z

code/src/ui/src/pages/atoms/ColorPaletteAtom.tsx

+// Utility function to determine the text color based on the brightness of the background color
+const getTextColor = (hex: string): string => {
+  hex = hex.replace('#', '');
+  const r = parseInt(hex.substring(0, 2), 16);


I'm not so sure but hex to substring is really slow and inefficient. The mathematical conversions applied are available via package. Also, the rounding up will make the maths inaccurate by 1-2 numbers (although I don't think it make have any effect). Have you tried the color package?

I've tried . It works good

Would you mind separating the #946 code from the #855 code and submit them in separate PRs off of separate branches? They really aren't related. And if, for some reason, we have to revert a commit related to one issue, then we aren't affecting unrelated issues.

aaronreed708

Also, please make sure that PRs that you make are against finos:dev, not finos:main. All changes go into the dev branch first. Thanks!

aaronreed708 · 2024-07-09T04:36:33Z

code/src/ui/src/pages/DesignSystemPage.tsx

@@ -1,8 +1,8 @@
-/*
+  /*


not sure why this space, but it makes license comment not line up. Please correct.

fixed it sir

aaronreed708 · 2024-07-09T04:41:41Z

code/src/ui/src/pages/WelcomePage.tsx

- * Copyright (c) 2023 Discover Financial Services
- * Licensed under Apache-2.0 License. See License.txt in the project root for license information
- */
-import React, { useEffect, useState } from 'react';


Not sure why the copyright header is being removed. If we need to address changing the copyright headers, we should do that under a separate issue.

aaronreed708 · 2024-07-09T04:46:56Z

code/src/ui/src/pages/atoms/ColorPaletteAtom.tsx

+// Utility function to determine the text color based on the brightness of the background color
+const getTextColor = (hex: string): string => {
+  hex = hex.replace('#', '');
+  const r = parseInt(hex.substring(0, 2), 16);


Would you mind separating the #946 code from the #855 code and submit them in separate PRs off of separate branches? They really aren't related. And if, for some reason, we have to revert a commit related to one issue, then we aren't affecting unrelated issues.

aaronreed708 · 2024-07-09T04:59:26Z

code/src/ui/src/pages/atoms/ColorPaletteAtom.tsx

+      </ExampleSection>
+    </div>
+  );
+};


I'm seeing a lot of changes here but not sure why. Are these all spacing changes? It is really hard to tell what has been changed on purpose and what is just spacing. Please make sure that the only changes showing here are changes that you intentionally made to fix the root issue or changes that you specifically mention in the PR. I don't think the changes here apply to either scenario.

I'm not sure why this was resolved, I still see changes here that are largely spacing related. It is difficult to tell what is specific to implementing new code for this PR and what isn't.

aaronreed708

Also, please make sure that PRs that you make are against finos:dev, not finos:main. All changes go into the dev branch first. Thanks!

aaronreed708 · 2024-07-09T05:02:22Z

Here is an unexpected behavior that I notice with your PR. The error reporting seems to be gone when the user changes the hex value to something invalid. Is this intentional?

aaronreed708 · 2024-07-09T05:04:09Z

Some more unexpected behavior. When I load a design system and then size the window smaller, the left nav disappears from the design system page but there is no hamburger icon with which to slide it out again.

aaronreed708 · 2024-07-09T05:05:16Z

The purple banner no longer stretches the whole application. Is this intentional?

AradhyaDixit18 · 2024-07-13T13:58:34Z

Fixed the width of the purple banner .

netlify · 2024-07-13T14:02:43Z

✅ Deploy Preview for glistening-gecko-6b417a ready!

Name	Link
🔨 Latest commit	`b85b9ef`
🔍 Latest deploy log	https://app.netlify.com/sites/glistening-gecko-6b417a/deploys/669289ffa4a06100085e960c
😎 Deploy Preview	https://deploy-preview-949--glistening-gecko-6b417a.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

AradhyaDixit18

@aaronreed708 fixed all the required changes please have a look at this

AradhyaDixit18 · 2024-07-13T14:02:39Z

code/src/ui/src/pages/DesignSystemPage.tsx

@@ -1,8 +1,8 @@
-/*
+  /*


fixed it sir

aaronreed708 · 2024-07-15T13:56:31Z

code/src/ui/src/pages/DesignSystemPage.tsx

@@ -105,7 +105,7 @@ const DesignSystemPage: React.FC<Props> = ({user, storage, themeName, setThemeNa

            // listen for changes in selected accessibility layers so that appropriate
            //  styles can be set
-            const layerChangeListener = function (event: EventValueChange<Boolean>) {
+              const layerChangeListener = function (event: EventValueChange<Boolean>) {


Why is this space added? Now beginning of line and ending bracket won't line up.

aaronreed708 · 2024-07-15T14:23:49Z

code/src/ui/src/pages/WelcomePage.tsx

+        window.addEventListener("resize", handleResize);
+        return () => window.removeEventListener("resize", handleResize);
+    }, []);
+


why this second setting of a resize even handler? Why wouldn't you put both pieces of logic in the same event listener if both pieces of logic need to happen on each resize?

aaronreed708 · 2024-07-15T14:30:12Z

code/src/ui/src/components/DesignSystemTitleBar.tsx

@@ -75,7 +75,7 @@ export const DesignSystemTitleBar: React.FC<Props> = ({ designSystemNames, desig

    return (
        <>
-            <div className="titleBarDiv" data-background="colored">
+            <div className="titleBarDiv " data-background="colored">
                <div className="left-titlebar">


why was this space added?

aaronreed708 · 2024-07-15T14:48:00Z

code/src/ui/src/pages/atoms/ColorPaletteAtom.tsx

+      </ExampleSection>
+    </div>
+  );
+};


I'm not sure why this was resolved, I still see changes here that are largely spacing related. It is difficult to tell what is specific to implementing new code for this PR and what isn't.

Sweetdevil144 · 2024-07-15T14:52:33Z

@AradhyaDixit18 . What @aaronreed708 is requesting is for you to apply prettier formatting once so that there isn't much confusion while reading the code. Sudden Spaces and jumps make the code look a bit confusing due to code quality issues :)

aaronreed708 and others added 3 commits June 7, 2024 09:30

Merge pull request finos#913 from finos/dev

0f3e393

Update main from dev finos#3

fixed the issues that i have listed in the issue 946

1b43e82

Made the design system page responsive

9e2a225

AradhyaDixit18 mentioned this pull request Jul 1, 2024

[TB] Enhance Responsiveness of ThemeBuilder Application on Mobile Devices #855

Open

Sweetdevil144 reviewed Jul 2, 2024

View reviewed changes

aaronreed708 requested changes Jul 9, 2024

View reviewed changes

aaronreed708 reviewed Jul 9, 2024

View reviewed changes

fixed navbar

cdd7a62

AradhyaDixit18 changed the base branch from main to dev July 13, 2024 13:47

fixed the purple banner issue

625141b

fixed all the issues

b85b9ef

AradhyaDixit18 commented Jul 13, 2024

View reviewed changes

code/src/ui/src/pages/DesignSystemPage.tsx Outdated

@@ -1,8 +1,8 @@

/*

/*

Copy link

Author

AradhyaDixit18 Jul 13, 2024

Choose a reason for hiding this comment

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

fixed it sir

aaronreed708 reviewed Jul 15, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Issue855. Make Main Landing Page Responsive and Add Hamburger Menu for Mobile View . #949

Issue855. Make Main Landing Page Responsive and Add Hamburger Menu for Mobile View . #949

AradhyaDixit18 commented Jul 1, 2024

netlify bot commented Jul 1, 2024 •

edited

Loading

Sweetdevil144 Jul 2, 2024

aaronreed708 Jul 9, 2024

Sweetdevil144 Jul 2, 2024

Sweetdevil144 Jul 2, 2024

AradhyaDixit18 Jul 2, 2024

aaronreed708 Jul 9, 2024

aaronreed708 left a comment

aaronreed708 Jul 9, 2024

AradhyaDixit18 Jul 13, 2024

aaronreed708 Jul 9, 2024

aaronreed708 Jul 9, 2024

aaronreed708 Jul 9, 2024

aaronreed708 Jul 15, 2024

aaronreed708 left a comment

aaronreed708 commented Jul 9, 2024

aaronreed708 commented Jul 9, 2024 •

edited

Loading

aaronreed708 commented Jul 9, 2024

AradhyaDixit18 commented Jul 13, 2024

netlify bot commented Jul 13, 2024 •

edited

Loading

AradhyaDixit18 left a comment

AradhyaDixit18 Jul 13, 2024

aaronreed708 Jul 15, 2024

aaronreed708 Jul 15, 2024

aaronreed708 Jul 15, 2024

aaronreed708 Jul 15, 2024

Sweetdevil144 commented Jul 15, 2024

		@@ -1,27 +1,6 @@
		{"k":"0000000080","o":"0000002870","v":"001"}

Issue855. Make Main Landing Page Responsive and Add Hamburger Menu for Mobile View . #949

Are you sure you want to change the base?

Issue855. Make Main Landing Page Responsive and Add Hamburger Menu for Mobile View . #949

Conversation

AradhyaDixit18 commented Jul 1, 2024

netlify bot commented Jul 1, 2024 • edited Loading

✅ Deploy Preview for dapper-ganache-45a60b ready!

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aaronreed708 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aaronreed708 left a comment

Choose a reason for hiding this comment

aaronreed708 commented Jul 9, 2024

aaronreed708 commented Jul 9, 2024 • edited Loading

aaronreed708 commented Jul 9, 2024

AradhyaDixit18 commented Jul 13, 2024

netlify bot commented Jul 13, 2024 • edited Loading

✅ Deploy Preview for glistening-gecko-6b417a ready!

AradhyaDixit18 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Sweetdevil144 commented Jul 15, 2024

netlify bot commented Jul 1, 2024 •

edited

Loading

aaronreed708 commented Jul 9, 2024 •

edited

Loading

netlify bot commented Jul 13, 2024 •

edited

Loading