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

Updated logo, hamburger icon, margins header and main. #577

Merged
merged 8 commits into from
Apr 1, 2025

Conversation

Martine04384
Copy link
Contributor

What does this PR do?

Updated logo and hamburger icon on sm and md screens
Updated margins in main between header and breadcrumbs and main/button and footer om sm and md screens.

Description of Task to be completed?

Compare and test.

How should this be manually tested?

Check in the browser and compare to Figma.

Any background context you want to provide?

A detected discrepancy between Figma and the file was detected after testing; an update was needed.

@Martine04384 Martine04384 linked an issue Mar 21, 2025 that may be closed by this pull request
@Martine04384 Martine04384 self-assigned this Mar 21, 2025
@inastefansdottir inastefansdottir linked an issue Mar 26, 2025 that may be closed by this pull request
@inastefansdottir inastefansdottir linked an issue Mar 26, 2025 that may be closed by this pull request
Copy link
Contributor

@inastefansdottir inastefansdottir left a comment

Choose a reason for hiding this comment

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

This looks fantastic, Martine! It's getting really close to matching the Figma file, great job!

I noticed that some links, like the logo, aren’t showing up due to the file structure changes. You can fix this by adding ../ to the logo path.

Also, I really like the hamburger menu you're using, but we need to follow the Figma design with pixel perfection. It would be great if you could update it to match the design.

That's all, everything else looks great! Keep up the awesome work!

Picture for reference, current changes on the branch is on the left and the figma design is on the right:
Screenshot 2025-03-28 003908 image

@Martine04384
Copy link
Contributor Author

Martine04384 commented Mar 30, 2025

Hi, thanks for the feedback!

I tried to fix the footer img on sm screens. Now its the same size as in figma nut a bit zoomed in. I kind of have to choose between the right look of the circles (as above) or the same size (as in this new code). The margins is updated so it looks better but not perfect.

I will have a look at the nav!

image

@Martine04384
Copy link
Contributor Author

Hi, seems like one of the hamburger icon "lines" got lost the last time. I've added the line and adjusted the first line to make it look more like in Figma.
image
image

The first img is how it looks like now and the second is the Figma design.

@inastefansdottir
Copy link
Contributor

Hi Martine its looking really great, I appreciate you making the changes that I requested! The two circles at the bottom look a lot better and more like the figma file. The hamburger menu looks pretty similar to the figma file with the extra line added, looking at it side by side. There are only tiny things that I noticed that could be improved for it to be perfect! The logo on the mobile version has too much space around it compared to the figma file, if you could lessen the space, that would be perfect. Left pic is live server and right is the figma file:

Screenshot 2025-03-31 183827 image

Also the logo on the mobile version is a bit too small compared to the figma file, could you make it a bit bigger to fit the figma file better? Left pic is live server and right is the figma file:

image Screenshot 2025-03-31 183752

Its looking so close to perfection!

@Martine04384
Copy link
Contributor Author

Hi, thanks for feedback! I have some problems with the logo header file. It can only be shown in 64px x 64px or 176px x 176px INCLUDED padding. I have tried everything (containers, scaling, transforming, positioning within a container), and I can't remove the padding with Tailwind. Therefore, I have exported the image file from Figma and added it to the image in our code. But I'm not sure about the quality of the image.

I also noticed a few lines off in the hamburger menu and fixed that.
And noticed the breadcrumbs where mt and mb 12 on lg screens and not 20 as in figma. But my tailwind stopped working again, so let me know if it worked! :)

@inastefansdottir
Copy link
Contributor

Hi Martine, The mobile version looks fantastic! It’s really close to the Figma file, and I don’t think any more changes are needed there. Great work!

Sorry for being so picky, but on the desktop version, the logo still feels a bit small compared to the Figma file, the padding looks fine on the logo, the size just needs to be increased a bit. Also the spacing around the breadcrumbs seems a bit too much. I know you worked on that, but since Tailwind stopped working, that might have caused some issues. If it happens again, you can try running npm run build to fix it. It is really odd that the mobile version has different inputs and text like the "positions" input and the "DOB" text than the desktop version, but I think it was a good idea to follow the mobile version.

image image

I also noticed that the margins inside the box needs to be increased according to the figma file, but I don't want to put too much work on you so I asked Adrian to make a bug issue for me to fix it.

Thanks again for your hard work!

@inastefansdottir
Copy link
Contributor

Hi again, I noticed that there is already a png img of the hamburger menu in the repo, could you use that one instead to keep consistency?

image

…main "box" and padding inside main, padding top h1, padding bottom button.
@Martine04384
Copy link
Contributor Author

Hi, okey now I am finally back with working Tailwind! Makes things a lot easier :D

I have fixed:

  • Logo desktop should be as big as in Figma.
  • Did a little mixup with my other branch so I have fixed the breadcrumbs again.
  • Switch the hamburger icon to img.
  • Fixed the size of the box in main and the padding inside this box.
  • I also added a bit more space over the H1 and under the button.

Hope this looks better!

Copy link
Contributor

@inastefansdottir inastefansdottir left a comment

Choose a reason for hiding this comment

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

This looks perfect, I appreciate you making all the changes I suggested. I am satisfied with the changes and happy to approve and merge the branch. Great work!

@inastefansdottir inastefansdottir merged commit dad3e27 into develop Apr 1, 2025
@inastefansdottir inastefansdottir deleted the 416-Fix-margins-and-logo-sm-screens branch April 1, 2025 13:54
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.

Fix navigation breaking on admin edit page
3 participants