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

enhancement for service section #592

Merged
merged 1 commit into from
Dec 16, 2024
Merged

Conversation

Devmoni
Copy link
Contributor

@Devmoni Devmoni commented Dec 15, 2024

Description:

Enhance responsiveness and styling of service section with equal-sized boxes, centered content, and consistent spacing.

Problem:

Icons in the service section were misaligned, and the spacing between elements was inconsistent. This led to a cluttered appearance and reduced responsiveness, impacting the user experience across different screen sizes.

Screenshot:

image

Testing:

Verified changes on a local Linux-based environment.

@Devmoni
Copy link
Contributor Author

Devmoni commented Dec 15, 2024

Sorry, my last PR was closed due to a force push, and I’m unable to recover it for now. I’ve created a new PR, and I assure you that this will not happen again.

@quozl
Copy link
Contributor

quozl commented Dec 15, 2024

Thanks. That's an improvement, but your commit message still does not follow our guidance. In particular;

  • start with a one line summary of the change;
  • leave a blank line after the summary; your message at the moment is all on one line,
  • explain the problem that is solved, unless the summary itself makes it obvious; and
  • use imperative mood, like "enhance", rather than "enhances".

Please rewrite your commit message (git commit --amend) and then force push (git push --force).

I'd also like you to consider the problems raised in #591 which came up because of your work on this section. I'm loath to accept this pull request given those issues, because it's like putting lipstick on a pig. Have you used Sugar? What happened when you visited the web site using Sugar?

@Devmoni
Copy link
Contributor Author

Devmoni commented Dec 15, 2024

I understand it's just a CSS update, and I’m keeping it separate because I’m exploring ways to solve the aforementioned issues while also trying out Sugar.
Regarding the mouse-over issue, it appears across the entire site, so I’m actively searching for a solution.

For now, I’ve updated the commit message.

@quozl
Copy link
Contributor

quozl commented Dec 15, 2024

Thanks. I checked one item in my reponse above.

You will note how GitHub has highlighted your commit message as violating convention;

msg

GitHub breaks up the message with ellipsis because you haven't followed the git convention of a single short one line summary followed by detail after a blank line.

You have already shown a difficulty with newlines. You can use a key marked enter or return to add newlines. Two newlines in a row is what we call a blank line.

Improved the layout for better visual appeal and responsiveness across the different screen sizes.
@Devmoni
Copy link
Contributor Author

Devmoni commented Dec 15, 2024

Thanks for the guidance and sorry for inconveniences.
Since I'm new to opensource contributions and git as well thats why i am making some silly mistakes :)

@quozl
Copy link
Contributor

quozl commented Dec 15, 2024

Thanks, change noted. @pikurasa your turn.

@pikurasa pikurasa merged commit 3de7dfd into sugarlabs:master Dec 16, 2024
@soumyaranjan-panda
Copy link
Contributor

I wanted to highlight that we should refrain from making custom modifications to the default Bootstrap grid system (such as changing the col-md- and col-sm- classes).

By modifying or overriding these classes, we risk breaking the built-in responsiveness, which could lead to inconsistent layouts across different screen sizes and make the codebase harder to maintain over time.

Instead, let’s stick with Bootstrap’s built-in grid classes. If specific layout adjustments are necessary, we can introduce additional custom styles or media queries without modifying the core grid system.

What are your thoughts on this, @quozl ?

@Devmoni
Copy link
Contributor Author

Devmoni commented Dec 17, 2024

But as mentioned before, all the new CSS should be written in custom.css.

@soumyaranjan-panda
Copy link
Contributor

But as mentioned before, all the new CSS should be written in custom.css.

Yes, new CSS should be written in custom.css. However, I wanted to point out that you made changes to the col-md- and col-sm- classes, which are part of Bootstrap's grid system. Modifying these classes directly can disrupt Bootstrap's built-in responsiveness and consistency. Instead, we should avoid altering these core classes and use custom styles or media queries in custom.css for any additional adjustments.

@Devmoni
Copy link
Contributor Author

Devmoni commented Dec 17, 2024

@soumyaranjan-panda
Thank you for pointing that out, and I completely understand your concern. The reason I made changes to the col-md- and col-sm- classes in custom.css was to address a specific issue I encountered while implementing responsiveness for the cards. The button was overflowing, and adjusting these classes seemed necessary to resolve it.

bhagyashree980 pushed a commit to bhagyashree980/www that referenced this pull request Dec 18, 2024
Improved the layout for better visual appeal and responsiveness across the different screen sizes.
bhagyashree980 pushed a commit to bhagyashree980/www that referenced this pull request Dec 18, 2024
Improved the layout for better visual appeal and responsiveness across the different screen sizes.
@quozl
Copy link
Contributor

quozl commented Dec 22, 2024

I wanted to highlight that we should refrain from making custom modifications to the default Bootstrap grid system (such as changing the col-md- and col-sm- classes).
...
What are your thoughts on this, @quozl ?

Thanks, good to know.

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.

4 participants