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

Split Layout to make code DRY #39

Merged
merged 1 commit into from
Oct 26, 2018
Merged

Split Layout to make code DRY #39

merged 1 commit into from
Oct 26, 2018

Conversation

niranjannitesh
Copy link
Member

@niranjannitesh niranjannitesh commented Oct 25, 2018

Please read and understand everything below
Do not delete any text other than where you are instructed.

Students: If one of them is applicable to you. Please check it.

Check by changing each [ ] to [x] Please take note of the whitespace as it matters.

  • Included a Preview link and screenshot showning after and before the changes.
  • Included a description of change below.
  • Squashed the commits.

Changes done in this Pull Request

Description / Changes

Splitted Header and Footer into separate files to make it DRY.

screenshot from 2018-10-25 19-08-18

@niranjannitesh niranjannitesh changed the title Split Layout Split Layout to make code DRY Oct 25, 2018
Copy link
Contributor

@fragm3 fragm3 left a comment

Choose a reason for hiding this comment

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

@niteshkumarniranjan Footer is good, header section has some changed css properties like background color, logo, font. Fix header.

@niranjannitesh
Copy link
Member Author

@fragm3 I have just removed a class thats why this is and about the logo I think this logo looks better in a single line rather than having two line logo.

@niranjannitesh
Copy link
Member Author

@fragm3 I have been doing UI/UX designs for more than a year and I think that having a light background adds contrasts so that people see the Banner first not the header.

@fragm3
Copy link
Contributor

fragm3 commented Oct 25, 2018

@niteshkumarniranjan The drop menu is of grey, I think grey is still better as most of the webpage is white, so grey adds contrasts. Not sure about the icon @mariobehling, thoughts?

@fragm3
Copy link
Contributor

fragm3 commented Oct 25, 2018

@niteshkumarniranjan It's better to stick to one issue per PR, you can create a separate issue, if you want to make other changes(UI/UX).

@niranjannitesh
Copy link
Member Author

niranjannitesh commented Oct 25, 2018

@fragm3 Reverted back to gray menu.

@fragm3
Copy link
Contributor

fragm3 commented Oct 25, 2018

@niteshkumarniranjan one PR for one issue. Make it exactly same, you can create new enhancement issues for improvements. Helps us keep track of work you have done, quick code review.

@niranjannitesh
Copy link
Member Author

@fragm3 I have reverted back to gray menu so there is only one issue being addressed.

@YashKumarVerma
Copy link
Member

What exactly does DRY Mean ?

@saarthakchats
Copy link
Member

Don't repeat yourself @YashKumarVerma

Copy link
Member

@saarthakchats saarthakchats left a comment

Choose a reason for hiding this comment

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

Why is there a picture of a boy for the favicon?

@niranjannitesh
Copy link
Member Author

niranjannitesh commented Oct 26, 2018

@saarthakchats that favicon wiil not appear in the website. that favicon is there because of my portfolio being hosted in by github.

@saarthakchats
Copy link
Member

Okay cool

Copy link
Member

@saarthakchats saarthakchats left a comment

Choose a reason for hiding this comment

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

Resolve conflicts.. rest looks good

@YashKumarVerma
Copy link
Member

Don't repeat yourself @YashKumarVerma

Couldn't get you

@saarthakchats
Copy link
Member

@YashKumarVerma
DRY means Don't Repeat Yourself the main aim of which is to reduce repetition of code.

Copy link
Member

@saarthakchats saarthakchats left a comment

Choose a reason for hiding this comment

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

Just squash your commits. LGTM

@niranjannitesh
Copy link
Member Author

@saarthakchats I cann't if I will squash now conflict will be back.

@tabesin
Copy link
Member

tabesin commented Oct 26, 2018

Why do you delete index html?

@niranjannitesh
Copy link
Member Author

niranjannitesh commented Oct 26, 2018

@tabesin I haven't deleted index.html

_includes/footer.html Show resolved Hide resolved
Copy link
Member

@liveHarshit liveHarshit left a comment

Choose a reason for hiding this comment

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

Squash all three commits to Split Layout Files (#16). And rebase it with upstream for getting merged.

@liveHarshit liveHarshit merged commit edd78b4 into fossasia:master Oct 26, 2018
@realslimshanky
Copy link
Member

@niteshkumarniranjan can you confirm this pr also closes #27 ?

@niranjannitesh
Copy link
Member Author

@realslimshanky Nope! That's another issue.

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.

None yet

9 participants