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

[Tidy] Replace html.Div with dbc.Card #373

Merged
merged 6 commits into from
Mar 14, 2024
Merged

Conversation

huong-li-nguyen
Copy link
Contributor

@huong-li-nguyen huong-li-nguyen commented Mar 14, 2024

Description

  • Replace html.Div with dbc.Card
  • Refactor CSS for the jumping effect on hover for navigation cards
  • Rename CSS classNames for outer card containers

Screenshot

Jumping effect previously disappeared and is now fixed again:

Screenshot 2024-03-14 at 12 50 52

Notice

  • I acknowledge and agree that, by checking this box and clicking "Submit Pull Request":

    • I submit this contribution under the Apache 2.0 license and represent that I am entitled to do so on behalf of myself, my employer, or relevant third parties, as applicable.
    • I certify that (a) this contribution is my original creation and / or (b) to the extent it is not my original creation, I am authorized to submit this contribution on behalf of the original creator(s) or their licensees.
    • I certify that the use of this contribution as authorized by the Apache 2.0 license does not violate the intellectual property rights of anyone else.
    • I have not referenced individuals, products or companies in any commits, directly or indirectly.
    • I have not added data or restricted code in any commits, directly or indirectly.

Copy link
Contributor

@petar-qb petar-qb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good. The only thing I'm not quite sure about is whether we want to add this to changelog.md

@huong-li-nguyen
Copy link
Contributor Author

huong-li-nguyen commented Mar 14, 2024

This looks good. The only thing I'm not quite sure about is whether we want to add this to changelog.md

Yeah, good question. I am not sure either, we could also just not mention it.
What do you guys think? @antonymilne @maxschulz-COL @petar-qb

  • Shall we mention the replacement of the underlying library in the components inside the changelog? (No strong opinion here)
  • Shall we mention CSS className changes in the changelog? (This could affect the user, but we already said that we don't consider it as breaking change, so I didn't mention it)

@maxschulz-COL
Copy link
Contributor

This looks good. The only thing I'm not quite sure about is whether we want to add this to changelog.md

Yeah, good question. I am not sure either, we could also just not mention it. What do you guys think? @antonymilne @maxschulz-COL @petar-qb

  • Shall we mention the replacement of the underlying library in the components inside the changelog? (No strong opinion here)
  • Shall we mention CSS className changes in the changelog? (This could affect the user, but we already said that we don't consider it as breaking change, so I didn't mention it)

Good question, and I am not sure my thoughts on this are always consistent. I would maybe propose this guideline:

  • directly affects user / would influence his decision to upgrade the package --> in changelog 100%
  • affects the user in more indirect ways (e.g. changing CSS class names), visuals change --> in changelog with a more generic mention (like "CSS property changes to X,Y,Z models/components" --> there are 2 reasons: users may use CSS properties, and visuals may change, both are nice if they are spottable whole looking at release notes)
  • anything else --> not in changelog

Now do I care about the returned components library type as a user? --> Generally no, but it could be a little bit like the CSS entry, some users may care if they use properties of the component?? Would maybe leave out.

In general we do not want to too lengthy changelogs, as they dilute focus.

Wdyt?

Copy link
Contributor

@maxschulz-COL maxschulz-COL left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@huong-li-nguyen
Copy link
Contributor Author

Wdyt?

Changing the underlying component library shouldn't have an effect on the user, as our model field arguments do not change. Visually they look the same more or less (not pixel perfect). So let me remove the changelog entry of that 👍

I could add an entry on the renaming of CSS classNames (this will happen more frequently though, but if we are fine with that - ok 👍 ). At some point this will all switch to the classNames that Bootstrap uses.

@huong-li-nguyen huong-li-nguyen enabled auto-merge (squash) March 14, 2024 12:59
@huong-li-nguyen huong-li-nguyen merged commit 61cb4fb into main Mar 14, 2024
33 of 34 checks passed
@huong-li-nguyen huong-li-nguyen deleted the tidy/replace_card branch March 14, 2024 13:07
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.

3 participants