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

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

Closed
wants to merge 2 commits into from

Conversation

@auge8472
Copy link
Contributor

auge8472 commented Apr 14, 2019

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

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

openttd-page-footer

@auge8472
Copy link
Contributor Author

auge8472 commented Apr 22, 2019

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.

Copy link
Member

LordAro left a comment

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 :)

margin-bottom: 20px;
text-align: center;

This comment has been minimized.

Copy link
@LordAro

LordAro May 2, 2019

Member

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

This comment has been minimized.

Copy link
@auge8472

auge8472 May 5, 2019

Author Contributor

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

@auge8472
Copy link
Contributor Author

auge8472 commented May 5, 2019

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

TrueBrain commented Dec 7, 2019

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?

@auge8472
Copy link
Contributor Author

auge8472 commented Dec 12, 2019

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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.