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

Feat/rounded klaro buttons #72

Open
wants to merge 5 commits into
base: deprecation
Choose a base branch
from

Conversation

ahrsjo
Copy link
Contributor

@ahrsjo ahrsjo commented Feb 26, 2025

AB#5212

Summary

Rounded button corners! 50rem is used elsewhere for the "pill" shape so I used the same value.

Turned out that we didn't need to make any changes in the Klaro repo.

Questions

  1. Updated the pipeline config in a separate commit included here. Should I keep it separate?
  2. How do we progress with the release from here? Should this be merged to branch deprecation and then on to main?
Screenshot 2025-02-26 at 16 45 03 Screenshot 2025-02-26 at 16 45 09

Copy link
Contributor

@mthege mthege left a comment

Choose a reason for hiding this comment

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

I've reviewed the style part of the PR, and for me it looks good locally. It feels a bit itchy that we don't use 1.25rem (like in kth/style), but it makes sense to follow klaro-css since Klaro doesn't use kth/style.

This is probably outside the scope, but I think the three buttons after clicking 'Manage services' are placed a bit awkward. Nothing have changed since before, but it is a bit more obivious now that they have a smaller area. But this might be fore another time.

Screenshot 2025-02-27 at 15 56 58

@ahrsjo
Copy link
Contributor Author

ahrsjo commented Feb 27, 2025

I've reviewed the style part of the PR, and for me it looks good locally. It feels a bit itchy that we don't use 1.25rem (like in kth/style), but it makes sense to follow klaro-css since Klaro doesn't use kth/style.

This is probably outside the scope, but I think the three buttons after clicking 'Manage services' are placed a bit awkward. Nothing have changed since before, but it is a bit more obivious now that they have a smaller area. But this might be fore another time.

Screenshot 2025-02-27 at 15 56 58

Thanks Martina! I agree but would like to see those changed as future improvements 🚀

@@ -52,7 +52,7 @@ stages:
- job: npm_publish
displayName: Publish NPM package
steps:
- template: ./npm-publish-beta.yml
- template: /templates/npm/publish.yml@stratus-templates
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 probably shouldn't have changed this template based on the config in main, but would be good with some verification.

Copy link
Contributor

Choose a reason for hiding this comment

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

This should probably have changed long ago!

./npm-publish-beta.yml is a local template in this repo, created before the cet-iac templates had support for releasing parallel versions (9 and 10 in this case).

If you managed to publish after making this change, then we should keep it. Also the file npm-publish-beta.yml can be removed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed

@@ -52,7 +52,7 @@ stages:
- job: npm_publish
displayName: Publish NPM package
steps:
- template: ./npm-publish-beta.yml
- template: /templates/npm/publish.yml@stratus-templates
Copy link
Contributor

Choose a reason for hiding this comment

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

Apparently I must also do a standalone comment to submit a review.
See answer above!

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