Skip to content

Conversation

@rmartins90
Copy link
Contributor

@rmartins90 rmartins90 commented Jan 16, 2025

Short description

Hover styles (text decoration and color) were only applied to the container element itself. This caused inconsistent behavior when the text link contained nested elements, as the hover styles wouldn't propagate to the children.

This PR uses .container:hover > * selector to target immediate children.

PR Checklist

  • Added tests for bugs / new features
  • Updated docs (storybooks, readme)
  • Executed npm run validate and made sure no errors / warnings were shown
  • Described changes in CHANGELOG.md
  • Bumped version in package.json and package-lock.json (npm --no-git-tag-version version <major|minor|patch>) ref
  • Reviewed and approved Chromatic visual regression tests in CI

Versioning

@rmartins90 rmartins90 changed the title fix: ensure TextLink styles are applied to children elements fix: ensure TextLink styles are applied to nested elements Jan 16, 2025
}

.container:hover {
.container > * {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not applying here other rules such as font-weight so they are preserved for nested elements

@rmartins90 rmartins90 self-assigned this Jan 16, 2025
@rmartins90 rmartins90 requested review from a team and nats12 and removed request for a team January 16, 2025 16:00
@rmartins90 rmartins90 marked this pull request as ready for review January 16, 2025 16:01
@rmartins90 rmartins90 force-pushed the rui/ensure-textlink-styles-are-applied-to-children-elements branch 6 times, most recently from ff11f5d to b08629c Compare January 16, 2025 23:16
Copy link
Contributor

@nats12 nats12 left a comment

Choose a reason for hiding this comment

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

Looks good Rui! Just one comment, with Reactist updates, we tend to keep the versioning changes separate from the product updates. So in this case, we would not make changes to the package.json and lock file or the CHANGELOG, we would do it in a separate PR.

Take one of the latest changes as an example:

@rmartins90
Copy link
Contributor Author

Looks good Rui! Just one comment, with Reactist updates, we tend to keep the versioning changes separate from the product updates.

That makes sense! We should then change the PR template, as it misleads. Do you agree? (see below)

  • Described changes in CHANGELOG.md
  • Bumped version in package.json and package-lock.json (npm --no-git-tag-version version <major|minor|patch>) ref

There are other examples where we change the version and CHANGELOG in the PR - d88972b

Either way, this process should be automated soon 🙂

@rmartins90 rmartins90 force-pushed the rui/ensure-textlink-styles-are-applied-to-children-elements branch from b08629c to 1f5eb2f Compare January 17, 2025 16:24
@rmartins90
Copy link
Contributor Author

Removed versioning from this PR @nats12. I'm merging this now.

@rmartins90 rmartins90 merged commit 391d00f into main Jan 17, 2025
5 checks passed
@rmartins90 rmartins90 deleted the rui/ensure-textlink-styles-are-applied-to-children-elements branch January 17, 2025 16:30
@nats12
Copy link
Contributor

nats12 commented Jan 21, 2025

That makes sense! We should then change the PR template, as it misleads. Do you agree? (see below)
There are other examples where we change the version and CHANGELOG in the PR - d88972b

Good point @rmartins90. @gnapse maybe can shed light on this: we are sometimes bumping versions in the same PR as the change, other times in separate PRs. Which process should we follow? ☺️

@gnapse
Copy link
Contributor

gnapse commented Jan 21, 2025

We've often bumped the version in the same PR as we make a product change (ref, ref, ref). But not always.

I don't think there's a hard rule on this. Personally, I tend to bump the version with the change, unless I have two or more changes that I know I can release together. In which case I hold off on the version bump, merge two or more PRs in a row, then do a PR with the version bump.

Regarding the PR request template, I think we can remove the "versioning" section regardless, because we already know from the PR title prefix if it's a patch release (e.g. fix:…), a minor release (e.g. feat:…) or a major release (breaking change).

@rmartins90
Copy link
Contributor Author

Meanwhile, I've created a PR to automate this process #863

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