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

Feature: add Tailwind CSS class 'rounded-2xl' #4153

Closed

Conversation

VenkaSri
Copy link
Contributor

Because

Adding border radius to course links on the landing page

This PR

  • added tailwind class 'rounded-2xl' to _curriculum.html

Issue

Related to #4142

Additional Information

Pull Request Requirements

  • [X ] I have thoroughly read and understand The Odin Project Contributing Guide
  • [ X] The title of this PR follows the keyword: brief description of change format, using one of the following keywords:
    • Feature - adds new or amends existing user-facing behavior
    • Chore - changes that have no user-facing value, refactors, dependency bumps, etc
    • Fix - bug fixes
  • [X ] The Because section summarizes the reason for this PR
  • [X ] The This PR section has a bullet point list describing the changes in this PR
  • [X ] I have verified all tests and linters pass after making these changes.
  • If this PR addresses an open issue, it is linked in the Issue section
  • If applicable, this PR includes new or updated automated tests

@KevinMulhern KevinMulhern temporarily deployed to odin-review-app-pr-4153 September 18, 2023 07:04 Inactive
@ManonLef ManonLef mentioned this pull request Sep 18, 2023
@ManonLef
Copy link
Member

ManonLef commented Sep 18, 2023

Thanks so much for doing this @VenkaSri !

While playing around on another PR here I started looking into our path layout, noticing they utilize rounded-lg on the path sections:
Screenshot 2023-09-18 at 20 59 18

I played around a bit and would love some opinions from you and @KevinMulhern on if that would be a better fit here:

rounded-lg

Screenshot 2023-09-18 at 21 58 48

rounded-2xl

Screenshot 2023-09-18 at 21 58 53

I hope I'm not a bother for I think the rounded-lg might be closer to what you initially proposed in your issue where I proposed keeping it consistent with the team cards PR. I know this is up for personal preference as well. I personally prefer the rounded-lg but would love to hear your input (or others reading this)

@KevinMulhern
Copy link
Member

rounded-lg gets my vote 🗳️

@VenkaSri
Copy link
Contributor Author

For my original proposal I had used rounded-md to ensure consistency with the all the other button links within the homepage, which all use the same border radius. I like both md and lg.

Also, the spacing of the h3 in middle card seems off cause of the neighbouring cards.
Screenshot 2023-09-18 at 10 33 09 PM

Made some minor changes to address that. What do you guys think? @ManonLef @KevinMulhern
Screenshot 2023-09-18 at 10 36 00 PM

@KevinMulhern
Copy link
Member

KevinMulhern commented Sep 19, 2023

Thanks @VenkaSri, theres not a lot between md and lg, but I think lg looks slightly better in this case.

I'm not sure about the centered text, can you take a screenshot of all the badges to see what it looks like within the overall section please?

@VenkaSri
Copy link
Contributor Author

Looks better imo, @KevinMulhern
Screenshot 2023-09-19 at 9 24 32 AM

@KevinMulhern
Copy link
Member

It looks decent to me, what do you think @ManonLef?

We'd probably be best reworking these components to have more horizontal space for longer titles, but thats getting way out of scope of what we're trying to do here. Something to do in a follow up maybe.

@KevinMulhern KevinMulhern temporarily deployed to odin-review-app-pr-4153 September 20, 2023 08:53 Inactive
@ManonLef
Copy link
Member

Yes I might like it better too, It makes the taller row less top heavy for JavaScript. It would need a lot more space to make it all fit horizontally. Definitely something we could look at in the future.

The changes are not showing on the deployment yet, so I suggest you commit the rounded-lg version with the proposed alignment changes @VenkaSri so we can review and approve it then!

@VenkaSri
Copy link
Contributor Author

Yes I might like it better too, It makes the taller row less top heavy for JavaScript. It would need a lot more space to make it all fit horizontally. Definitely something we could look at in the future.

The changes are not showing on the deployment yet, so I suggest you commit the rounded-lg version with the proposed alignment changes @VenkaSri so we can review and approve it then!

Done :)

KevinMulhern pushed a commit that referenced this pull request Sep 22, 2023
<!-- Thank you for taking the time to contribute to The Odin Project. In
order to get this pull request (PR) merged in a reasonable amount of
time, you must complete this entire template. -->

## Because
<!-- Summarize the purpose or reasons for this PR, e.g. what problem it
solves or what benefit it provides. -->
Aligning course titles in the course listing cards. 

## This PR
<!-- A bullet point list of one or more items describing the specific
changes. -->

- added tailwind class 'rounded-lg' to link
- added `flex grow items-center` to the headings

## Issue
<!--
If this PR closes an open issue in this repo, replace the XXXXX below
with the issue number, e.g. Closes #2013.

If this PR closes an open issue in another TOP repo, replace the #XXXXX
with the URL of the issue, e.g. Closes
https://github.com/TheOdinProject/curriculum/issues/XXXXX

If this PR does not close, but is related to another issue or PR, you
can link it as above without the 'Closes' keyword, e.g. 'Related to
#2013'.
-->
Related to #4153 

## Additional Information
<!-- Any other information about this PR, such as a link to a Discord
discussion. -->


## Pull Request Requirements
<!-- Replace the whitespace between the square brackets with an 'x',
e.g. [x]. After you create the PR, they will become checkboxes that you
can click on. -->
- [ X] I have thoroughly read and understand [The Odin Project
Contributing
Guide](https://github.com/TheOdinProject/theodinproject/blob/main/CONTRIBUTING.md)
- [X ] The title of this PR follows the `keyword: brief description of
change` format, using one of the following keywords:
    - `Feature` - adds new or amends existing user-facing behavior
- `Chore` - changes that have no user-facing value, refactors,
dependency bumps, etc
    - `Fix` - bug fixes
-   [ X] The `Because` section summarizes the reason for this PR
- [ X] The `This PR` section has a bullet point list describing the
changes in this PR
- [X ] I have verified all tests and linters pass after making these
changes.
- [ ] If this PR addresses an open issue, it is linked in the `Issue`
section
-   [ ] If applicable, this PR includes new or updated automated tests
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants