Improved view: welcome_message.php

[Vizzielli]

Description
Various changes, solved some w3 validator warnings and erros, edited js for mobile menu (using toggle instead of if/else), some margin/padding and color adjustment about mobile navigation and a bit of css transitions for css hover effects. Cursor pointer on toggle menu button/link. I think both validator and graphic changes improve the current one. Fixed reverse menu items on mobile or desktop version.

Checklist:

• Check done on https://validator.w3.org/
• Check done on https://jigsaw.w3.org/css-validator/
• Check done on https://search.google.com/test/mobile-friendly

Update V3:

• converted toggle menu link to button
• hover colors change mainly in menu and footer like codeigniter.com ones (better contrast and readability)
• css focus added (navigate using tab - accessibility)
• a bit bigger logo (setup using height only, its adaptive, removed width attribute)
• quotes around font names
• lang attribute back

[LittleJ]

Thank you for the suggestions, but:
1/ This is my advice: you shouldn't take W3C recommandations as the Bible, these are just recommandations, not requirements ;-) For instance, they want you to use href="#" though it will add an anchor to the url, which can be interpreted by JS scripts (sometimes for uri routing). That's how people end up using tricks like "javascript:void" on their links.
2/ What are exactly the other changes ? Except adding 'lang="en"' too, which is not a bad idea but is not required (like the other global attributes: https://developer.mozilla.org/en-US/docs/Web/HTML/Global_attributes). The pull request shows 203 lines deleted and 238 created. Did you replace spaces by tabs by mistake when you copy-pasted ? W3C does not recommend this especially :-)

Do not mistake yourself, I am open to any suggestion. But I take a lot of time to write and review each line of code I produce, so I like to discuss and understand precisely what is wrong or could be improved. Even spaces and tabs :-)

[Vizzielli]

Thank you for the suggestions, but:

Thanks for the feedback!

1/ This is my advice: you shouldn't take W3C recommandations as the Bible, these are just recommandations, not requirements ;-) For instance, they want you to use href="#" though it will add an anchor to the url, which can be interpreted by JS scripts (sometimes for uri routing). That's how people end up using tricks like "javascript:void" on their links.

Ok, I considered a forum comment but you're right, I removed # on toggle. BUt I think it may be ok for the home link.

2/ What are exactly the other changes ? Except adding 'lang="en"' too, which is not a bad idea but is not required (like the other global attributes: https://developer.mozilla.org/en-US/docs/Web/HTML/Global_attributes). The pull request shows 203 lines deleted and 238 created. Did you replace spaces by tabs by mistake when you copy-pasted ? W3C does not recommend this especially :-)

Removed lang, thanks!
About edits, I tried to go back to the previous indentation, let me know if it is better. About edits:

• Removed duplicated alt on img
• Moved style to head
• Reduced js
• Removed menu reverse that I consider not intendeed + graphic changes (before desktop https://prnt.sc/r2664x mobile https://prnt.sc/r266i0), than (desktop https://prnt.sc/r267d7 mobile https://prnt.sc/r268zd ), so it start with home in both versions.
• And about graphics, it may be better or worse, it is more subjective thing I can revert.
• Others on the top.

Do not mistake yourself, I am open to any suggestion. But I take a lot of time to write and review each line of code I produce, so I like to discuss and understand precisely what is wrong or could be improved. Even spaces and tabs :-)

No problem, I want suggestions as your ones, in any case. Thank again!

[LittleJ]

OK, thanks ! I think it’s better when you explain things like this. And choosing the same indentation makes it easier to read changes.

My comments:

1/ Removed duplicated alt on IMG… All right :-)
2/ Moved style to head… I know, I thought the same. But the previous template was designed like this. So I thought it might be a deliberate choice from the team. I let @lonnieezell decide :-)
3/ Reduced JS… Why not. I just thought at that time that it would be easier for beginners to understand.

I have to split your next point, because these are separated things in fact…

4/ Removed menu reverse… I noticed it. But I thought it was not important, and doesn’t worth more development/code. You did it. Why not.
5/ Breakpoints… I chose to use only one. Because I think only one is required, after they all share the same properties. This adds unnecessary code. I let @lonnieezell decide :-)
6/ CSS transitions… Good idea ;-)
7/ For the toggle link... ** There is still the issue of the anchor added to the url. We can remove « href » as I did, we can let it like this, we can change the JS script so that it listens to clicks and it could maybe not add the anchor with a « preventDefault() ». I let @lonnieezell decide :-)**

[LittleJ]

Please add @lonnieezell to reviewers on top right of your pull request ;-)

[Vizzielli]

2/ Moved style to head… I know, I thought the same. But the previous template was designed like this. So I thought it might be a deliberate choice from the team. I let @lonnieezell decide :-)

Ah ok, I hadn't considered the previous style. Let's wait, maybe we can learn something new about this or at least understand why.

3/ Reduced JS… Why not. I just thought at that time that it would be easier for beginners to understand.

Yes, surely first one was easier but we can avoid about 10+ lines here if you count js and css. And I think it would be good if someone approach to new code, I learn a lot using CI, this could be useful for others too. At the end, it isn't that hard too.

5/ Breakpoints… I chose to use only one. Because I think only one is required, after they all share the same properties. This adds unnecessary code. I let @lonnieezell decide :-)

Yeah, a lot subjective. I just thought about reducing code if/where it is possible.

7/ For the toggle link... ** There is still the issue of the anchor added to the url. We can remove « href » as I did, we can let it like this, we can change the JS script so that it listens to clicks and it could maybe not add the anchor with a « preventDefault() ». I let @lonnieezell decide :-)**

Maybe im looking in wrong dir, but I cant see it now on toggle: http://prntscr.com/r2al0f
https://github.com/GianVizzielli/CodeIgniter4/blob/patch-2/app/Views/welcome_message.php

Please add @lonnieezell to reviewers on top right of your pull request ;-)

Done, thank you. I'm like newbie to do things on Github, need to learn it, step by step. :D

[John-Betong]

For what it is worth I have uploaded the source code to :

https://ci4-strict.tk/welcome_new.

I think that any validation errors or warnings reflect upon the CodeIgniter4 Project and cast serious doubt upon attention to detail.

I also think that:

1. W3.org validation recommendations should be followed apart from using a pound sign. I prefer using a question mark and have added to the online demo.

2. I have also used quotes around font names containing a space to eliminate warnings.

3. There are still six CSS warnings related to font-smoothing, webkit and an Opera "-o-transition" reference none of which I think are necessary.

4. I don't see why there is no lang attribute, the site is for an English audience. I believe that not only would the warning be removed it would also assist slightly with SEO by eliminating non-English languages.

5. I do not see why the JavaScript type attribute is being used when I believe HTML5 specification made it unnecessary.

[off-topic]
Why is log "data:image/png;base64" being used and not the compact SVG replacement?

Also there was talk quite some time ago about using the Google Raleway font although I think this would add an unnecessary overhead.
[/off-topic]

[LittleJ]

Where were you guys when we were working on this the previous days ? 😀🙃

1/ Sorry, I’m french. I did not understand or followed you on this. And what do you mean by « cast serious doubt upon attention to details » ?
2/ This is copy-pasted from Chrome’s console, coming from Github’s website. Not sure quotes are required.
3/ Old and specific browsers optimizations are never a bad thing to me. It costs nothing. Webkit prefixes are standard.
4/ When not defined, the language is auto-detected by Google. Plus you define your audience where you submit your site, in Google Analytics. Anyway, I did not say don’t use it, i just said it is ok but not required. We can add it of course.
5/ To check. Anyway this is not an issue.

For graphics, we use what we have. What we find on the internet. There is no official repo with assets (I believe). That’s actually a good question because I was thinking about suggesting a « Press » section on the website, so that people get the official graphic charter and logos...

Regarding the font-stack, as I said, it is Github’s one (sans-serif). On the other project (future website) they use another Google font. I chose not to include any external source as it is a constraint for this page. No external image, stylesheet, font... this is the reason 🙂 It has to work locally, without any internet connection.

[Vizzielli]

This broke the navigation to me, page reload on clic, menu open and close instantly for the reload (chrome/brave on desktop + android). Maybe better, at this point, to convert it to button or just generic div at all, so we don't have to put href for any reason...

Not seen this before, I agree, also if it works without it.

I prefer lonnieezell reply about this. Clearly all vendor attributes isn't really necessary here.

Sure, I use it always and it give no problem also if isnt needed, For the moment I think to put it back, than wait for lonniee comments..

I agree, has been removed in this proposal yet.

Here demo of last changes:
https://jsfiddle.net/dw9o7qmf/

[lonnieezell]

2. I've always used quotes around font names with spaces. Not sure what the current best practices. I've spent the last 4 years barely being allowed to do front end work lol.
3. I have no problem with the font-smoothing ones. -webkit-transition and -o-transition look to be deprecated and not recommended anymore.
4. Keep lang in html tag.
5. I would remove the extra type to the script tag. The page is obviously written in HTML5 which recommends doing away with that.

Why is log "data:image/png;base64" being used and not the compact SVG replacement?

I'm not familiar with the replacement you're talking about. Typically SVG requires a vector version of the file that I am not sure we have access to.

Also there was talk quite some time ago about using the Google Raleway font although I think this would add an unnecessary overhead.

I agree with not using external resources here.

[Vizzielli]

I think it is updated as received suggestions.
Let me know if needed any other adjustment. Thank you all for the feedback.

[lonnieezell]

Looks good to me. Thanks!

Merging.