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.

Sign up for GitHub

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

Jump to bottom

Change: semantic HTML-elements for the pages main sections and corresponding CSS-rules #84

Closed
wants to merge 2 commits into from
Closed

Change: semantic HTML-elements for the pages main sections and corresponding CSS-rules #84

wants to merge 2 commits into from
+32 −30

Conversation

auge8472
Copy link
Contributor

@auge8472 auge8472 commented Apr 14, 2019 •
edited
Loading

Changed HTML

  • changed div#header into header#header
  • changed div#navigation into nav#navigation
  • changed div#content-main into main#content-main
  • changed div#content-bottom into footer#content-bottom and separated it from main

Changed CSS

  • removed the background image from body and replaced it by a background colour
  • changed selector for #header into body > header (combination because a page can contain several elements header (i.e. further header for blog articles))
  • changed selector for #navigation into body > nav (combination because a page can contain more than one elements nav)
  • changed selector #content-main into main ("There can be only one")
    • centered the link to older blog articles below the blog article section without the use of float because float extracts the element from the page element flow
  • changed selector #content-bottom into body > footer (combination because a page can contain several elements footer (i.e. further footer for blog articles))
    • because of the separation of the footer from the main block the footer has now a separate background with rounded corners and box shadow similar to main
  • kept the IDs because I can't ensure, that they are not addressed from elsewhere (i.e. in CSS-selectors)

@auge8472
Copy link
Contributor Author

auge8472 commented Apr 14, 2019 •
edited
Loading

As an addition a screenshot from the draft for the footer.

openttd-page-footer

@auge8472
Copy link
Contributor Author

Is there anyone? Everybody in the Easter holidays? ;-)

I hoped to get a reaction to satisfy myself to be on the right way. Same for #85.

LordAro
LordAro requested changes May 2, 2019
View reviewed changes
Copy link
Member

@LordAro LordAro left a comment

Choose a reason for hiding this comment

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

Sorry for the delay in getting to this

Just the one minor nitpick. I feel like I'd prefer it if the "unnecessary" ids were removed as well - what did you mean by "I can't ensure, that they are not addressed from elsewhere" ? Seems like something a text search can deal with :)

static/css/page.css
margin-bottom: 20px;
text-align: center;
Copy link
Member

Choose a reason for hiding this comment

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

I feel like this change might be confusing, given the name of the class. The class is only used in 2 places - rename?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Seems to be very plausible to change the class name. :-)

@auge8472
Copy link
Contributor Author

auge8472 commented May 5, 2019 •
edited
Loading

Just the one minor nitpick. I feel like I'd prefer it if the "unnecessary" ids were removed as well - what did you mean by "I can't ensure, that they are not addressed from elsewhere" ? Seems like something a text search can deal with :)

I'll search the sources. If I'll find something I can't comprehend I'll ask here. Otherwise I'll add a commit to remove the ids.

@TrueBrain
Copy link
Member

Ping @LordAro : three tickets with near identical suggested changes, but no activity in a long time. What do you want to do with the pull requests?

This was referenced Dec 7, 2019
Closed
Merged
@auge8472
Copy link
Contributor Author

After pushing the task more than a half year along, I came into trouble because of the many changes in the meantime. Because of merging of these changes into my branch it came from three to around 20 commits. I tried to squash this into one commit but that would reintroduce all your changes a second time. So I decided to close this PR and opened a new one (#113) with only my own changes (cherry-picked and squashed).

@auge8472 auge8472 closed this Dec 12, 2019
@auge8472 auge8472 deleted the maintemplate branch June 6, 2023 20:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@LordAro LordAro

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@auge8472 @TrueBrain @LordAro