Skip to content

OS: 41, 47, 48: Create Hero, Text, Two Column in Storybook#4

Open
shayaanmiyy wants to merge 10 commits into
Cardfrom
htmldesign
Open

OS: 41, 47, 48: Create Hero, Text, Two Column in Storybook#4
shayaanmiyy wants to merge 10 commits into
Cardfrom
htmldesign

Conversation

@shayaanmiyy

@shayaanmiyy shayaanmiyy commented Jul 1, 2024

Copy link
Copy Markdown
Collaborator

Jira Ticket URL: https://digitalpolygon.atlassian.net/browse/OS-47

Decription

Create the Sections Component from https://html5up.net/uploads/demos/forty/

@niteshsyngenta11 niteshsyngenta11 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Incorporate the Feedback.

In general:

  • Line space missing
  • Use shorthand
  • Use color variables instead of using it directly.

const twigAlias = {
"@atoms": path.resolve(__dirname, '../', 'components/atoms'),
"@molecules": path.resolve(__dirname, '../', 'components/molecules'),
"@molecules": path.resolve(__dirname, '../', 'components/molecule'),

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This should be molecules.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Is because I am unable to change the Name of the folder, some restrictions are there

image

image

$portage: #8ea9e8;
$de-york: #87c5a4;
$ebony-clay: #242943;
$martinique: #2a2f4a; No newline at end of file

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Add an extra line space.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Okay Done

Comment thread web/app/themes/dp-timber-theme/components/style.scss
.card-container {
max-width: 600px;
}
} No newline at end of file

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Add an extra line space.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Yes I added one Line space

}
}

export const Default = {} No newline at end of file

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Add an extra line space.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Yes

width: calc(100% - 6em);

@media screen and (max-width: 736px) {
padding: 3em 0 1em 0;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Use shorthand.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Yes for Margin and padding I am using shorthand


header> :first-child:after {
content: '';
background-color: #ffffff;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Use variables.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Got It, I corrected my code

background-color: #ffffff;
display: block;
height: 2px;
margin: 0.325em 0 0.5em 0;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Use shorthand.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Done

font-size: 0.7em;
font-weight: 600;
letter-spacing: 0.25em;
margin: 0 0 2em 0;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Use shorthand

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Done

color: var.$white;
font-weight: 600;
line-height: 1.65;
margin: 0 0 1em 0;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Use shorthand

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Done

@shayaanmiyy shayaanmiyy changed the title Html Design in Storybook OS: 41, 47, 48: Create Hero, Text, Two Column in Storybook Jul 4, 2024

@niteshsyngenta11 niteshsyngenta11 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Incorporate the feedback.

@@ -0,0 +1,6 @@
$sm: "480px";

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The name of the folder should be scss not SCSS.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Yes, I renamed the folder to lower case scss

filter: blur($blur);
}


Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Remove the extra line space.

&:hover {
background-color: var.$anakiwa;
}
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Fix the indentation issues. It should be 2 spaces instead of a tab

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Corrected Indentation for all


{{ text }}

{% if text == "Get Started" %}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Why have we added this condition?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

add a new variable to show svg only if the Boolean is set to true

@@ -0,0 +1,9 @@
<button class="get-started {{ fill ? 'fill' : '' }} {{ width | default('w-100') }}">

{{ text }}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Fix the indentation issue.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Fixed

Comment on lines +54 to +62
>.split>* {
padding: 3em 0 1em 0;
}

>.split> :first-child:before {
display: block;
}
}
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This can be clubbed.

width: calc(100% - 3em);
}
}
} No newline at end of file

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Line space missing.

Comment on lines +54 to +69
header {
.major {
width: max-content;
margin-bottom: 2em;
}
}

header.major> :first-child:after {
content: '';
background-color: var.$white;
display: block;
height: 2px;
margin: 0.325em 0 0.5em;
width: calc(100% + 0.6em);
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This can be clubbed.

<h1 class="d-inline">{{ title }}</h1>
</header>
<span class="image main mt-4 mb-4">
{% include '@atoms/image/image.twig' with { src: 'https://html5up.net/uploads/demos/forty/images/pic11.jpg', alt: 'Generic', classes: 'img-fluid', width: '100%', height: 'auto' } %}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Make sure this is divided into multiple lines.

Comment on lines +79 to +105
.hero {
padding: 5em 0 1em;
height: auto;
margin-bottom: -2.75em;
max-height: none;
min-height: 0;
top: -2.75em;
}

.hero h1 {
font-size: 2em;
}

.hero>.inner .content {
display: block;
}

.hero>.inner .content>* {
margin-right: 0;
margin-bottom: 2em;
}

.hero.major {
height: auto;
min-height: 0;
max-height: none;
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We are using SCSS so we should not write any CSS.

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