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

Increase the top height on the chart of space usage #5

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Increase the top height on the chart of space usage #5

wants to merge 2 commits into from

Conversation

SupianIDz
Copy link

Hi, thank you for creating this cool template, but I think it would be better if the height of the space usage chart was increased a little. Because it is too close to the username.

BEFORE
before

AFTER
after

Thanks

Copy link
Owner

@thexdev thexdev left a comment

Choose a reason for hiding this comment

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

Hi, I've reviewed your commit change. Please complete the request changes and I'll be ready to merge your commit change :)

@@ -3,7 +3,7 @@ import Chart from "assets/img/illustration/chart.svg";

const SpaceUsage = () => {
return (
<div className="flex flex-wrap items-end content-end justify-center mt-6 relative h-32">
<div className="flex flex-wrap items-end content-end justify-center mt-6 relative h-40">
Copy link
Owner

Choose a reason for hiding this comment

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

You don't need to change the height. It will break the UI when the screen resolution is lower than 1920 x 1080.

Suggested change
<div className="flex flex-wrap items-end content-end justify-center mt-6 relative h-40">
<div className="flex flex-wrap items-end content-end justify-center mt-6 relative h-32">

@@ -3,7 +3,7 @@ import Chart from "assets/img/illustration/chart.svg";

const SpaceUsage = () => {
return (
<div className="flex flex-wrap items-end content-end justify-center mt-6 relative h-32">
<div className="flex flex-wrap items-end content-end justify-center mt-6 relative h-40">
<img src={Chart} alt="Space usage" className="absolute w-full" />
Copy link
Owner

Choose a reason for hiding this comment

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

Just add class h-full after w-full and the image will perfectly fit on every screen.

Suggested change
<img src={Chart} alt="Space usage" className="absolute w-full" />
<img src={Chart} alt="Space usage" className="absolute w-full h-full" />

purge: ['./src/*.js', './src/components/**/*.js'],
theme: {
extend: {},
height: {
Copy link
Owner

Choose a reason for hiding this comment

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

And now you no longer need to change the config. Just remove this commit and I'll be ready to merge your commit change :)

@thexdev
Copy link
Owner

thexdev commented Aug 9, 2020

Hi, thanks for the correction. It looks like something like this happens on screens that are equal to or larger than 1920 x 1080. This is caused by the image overflowing of the wrapper.

I've reviewed your commit change and I think you shouldn't need to change the height because this problem comes from the image not from the wrapper.

If you don't mind you can add a class h-full to the image instead of increasing the height of the wrapper so the image doesn't overflow from the wrapper and fit to every screen size.

And please use commit convention (semantic commit) so the commit changes in this repository look consistent. You can also use gitmoji to describe what you have done.

These links might helpful:

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.

2 participants