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

Fixes #545 text color of "about us" option and "news" option #565

Merged
merged 2 commits into from
Dec 2, 2024

Conversation

therealharshit
Copy link
Member

Fixes #545
This is how it look like
dropdown
dropdown 2

@quozl @pikurasa please review.

@quozl
Copy link
Contributor

quozl commented Nov 30, 2024

Thanks. Reviewed.

  • does not match the Bootstrap example for the version in use, in that the immediately surrounding element is a ul with class dropdown-menu, instead of a div with class dropdown-menu.

Can you explain why you have chosen ul?

Reference: https://getbootstrap.com/docs/4.0/components/navbar/

@therealharshit
Copy link
Member Author

Thanks. Reviewed.

  • does not match the Bootstrap example for the version in use, in that the immediately surrounding element is a ul with class dropdown-menu, instead of a div with class dropdown-menu.

Can you explain why you have chosen ul?

Reference: https://getbootstrap.com/docs/4.0/components/navbar/

@quozl I haven't used <ul> intentionally I just don't want to make extra changes to the code in order to fix this issue.
I have refactored the code and everything is working the way it should, please review.

@quozl
Copy link
Contributor

quozl commented Nov 30, 2024

Thanks. Reviewed. Looked at history; was this problem introduced by 7088c6d? Updated issue.

@therealharshit
Copy link
Member Author

Thanks. Reviewed. Looked at history; was this problem introduced by 7088c6d? Updated issue.

@quozl No it wasn't that commit aims to resolve the responsiveness of the navbar.
You can see in that commit the dropdown-menu tags are already <li> and <ul>,
And the code which I added for navbar of mobile devices is perfect.

@quozl
Copy link
Contributor

quozl commented Nov 30, 2024

Thanks. I've dug deeper, the practice was in the original GSoC 2017 commit 725368d, with Bootstrap 3.3.7, for which the documentation is not easily available, but in tag v3.3.7 file docs/examples/navbar/index.html the practice is clearly present.

Therefore our upgrade to Bootstrap 4.0.0 in 99eabc0 may have been incomplete.

@Ajaykumar724
Copy link

Fixes #545
This is how it look like
dropdown
dropdown 2

@quozl @pikurasa please review.

This is OK
But navbar text color blue will very expressive

@therealharshit
Copy link
Member Author

@quozl Is the review process completed? or anything is left to be fixed.

@quozl quozl merged commit 7dbca4a into sugarlabs:master Dec 2, 2024
@quozl
Copy link
Contributor

quozl commented Dec 2, 2024

Thanks for your patience. Delay was due to contributions to the issue by others who wanted to fix the problem, but in the end didn't want the problem fixed enough to help us fix it. I sought consensus but there was no useful response within a reasonable time.

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.

text color of "about us" option and "news" option
3 participants