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

Removed the border-radius on align left #754

Merged
merged 8 commits into from
Aug 27, 2024

Conversation

Spoorthy1423
Copy link
Contributor

#741 Removed border-radius on align left
Removed unused sections and

Removed border-radius when the thumbnail display is align left using thumbnailAlign prop and made thumbnail prop large.

Description

Issue addressed

Addresses #PR# HERE

Before/after screenshots

Screenshot 2024-08-24 at 6 43 33 PM

Changelog

  • [#PR no]
    • Description: Summary of change(s)
    • Products impact: Choose from - none (for internal updates) / bugfix / new API / updated API / removed API. If it's 'none', use "-" for all items below to indicate they are not relevant.
    • Addresses: Link(s) to GH issue(s) addressed. Include KDS links as well as links to related issues in a consumer product repository too.
    • Components: Affected public KDS component. Do not include internal sub-components or documentation components.
    • Breaking: Will this change break something in a consumer? Choose from: yes / no
    • Impacts a11y: Does this change improve a11y or adds new features that can be used to improve it? Choose from: yes / no
    • Guidance: Why and how to introduce this update to a consumer? Required for breaking changes, appreciated for changes with a11y impact, and welcomed for non-breaking changes when relevant.

[#PR no]: PR link

Steps to test

  1. Step 1
  2. Step 2
  3. ...

(optional) Implementation notes

At a high level, how did you implement this?

Does this introduce any tech-debt items?

Testing checklist

  • Contributor has fully tested the PR manually
  • If there are any front-end changes, before/after screenshots are included
  • Critical and brittle code paths are covered by unit tests
  • The change is described in the changelog section above

Reviewer guidance

  • Is the code clean and well-commented?
  • Are there tests for this change?
  • Are all UI components LTR and RTL compliant (if applicable)?
  • Add other things to check for here

After review

  • The changelog item has been pasted to the CHANGELOG.md

Comments

@MisRob MisRob requested a review from AllanOXDi August 26, 2024 09:58
@MisRob
Copy link
Member

MisRob commented Aug 26, 2024

@Spoorthy1423 Thank you - if you can, it'd be a bit easier if you just pushed new commits to already open pull request rather than closing after every feedback, so we can follow what happened and why. I will loop in @AllanOXDi for review here - Allan, also please note #753 (comment)

@@ -320,7 +320,7 @@
},
thumbnailStyles: {
...thumbnailCommonStyles,
borderRadius: '8px 0 0 8px',
borderRadius: '0 8px 8px 0',
Copy link
Member

Choose a reason for hiding this comment

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

Hi @Spoorthy1423 . Thanks for you work here. I have reviewed it and I think it's still breaking when it's align left.
Screenshot 2024-08-26 at 16 05 16

Copy link
Member

Choose a reason for hiding this comment

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

I guess we will need to swap it when the thumbnail is on the right
borderRadius: '0 8px 8px 0',

And when the thumbnail is on the left
borderRadius: '8px 0 0 8px',

Copy link
Member

Choose a reason for hiding this comment

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

Once we confirm that everything is looking good we will go a head and merge it.

Copy link
Member

@AllanOXDi AllanOXDi left a comment

Choose a reason for hiding this comment

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

Hi @Spoorthy1423 Thanks for updating the logic . I just want to note that the linting check is failing. Could you run pre-commit as described in this section of the developer documentation? and commit it again.
I will let my colleague @MisRob to give it a second eye and we merge

@Spoorthy1423
Copy link
Contributor Author

@AllanOXDi i am actually facing this issue

(myenv) spoorthymadasu@spoorthys-MacBook-Air kolibri-design-system % pre-commit run --all-files
An error has occurred: InvalidConfigError:
=====> .pre-commit-config.yaml is not a file
Check the log at /Users/spoorthymadasu/.cache/pre-commit/pre-commit.log

do i need to create a pre-commit-config.yaml

@AllanOXDi
Copy link
Member

Sorry could you try yarn lint -w

@AllanOXDi
Copy link
Member

I guess you will have to dopre-commit installfirst

@AllanOXDi
Copy link
Member

AllanOXDi commented Aug 27, 2024

Hi @Spoorthy1423 I am going to reopen this PR and close the one you just opened. Reason being that this one already have the reviews

@AllanOXDi AllanOXDi reopened this Aug 27, 2024
@AllanOXDi
Copy link
Member

AllanOXDi commented Aug 27, 2024

As for the failing lint check. I have just fixed that and it should pass. I will wait for my colleague to give it a final look and we will proceed to marge it.

@AllanOXDi AllanOXDi requested a review from MisRob August 27, 2024 07:16
CHANGELOG.md Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
CHANGELOG.md Show resolved Hide resolved
@MisRob
Copy link
Member

MisRob commented Aug 27, 2024

Thanks both. Just few changelog tweaks to help the release manager and should be good to go.

@MisRob MisRob mentioned this pull request Aug 27, 2024
8 tasks
Spoorthy1423 and others added 5 commits August 27, 2024 17:45
Co-authored-by: Michaela Robosova <[email protected]>
Co-authored-by: Michaela Robosova <[email protected]>
Co-authored-by: Michaela Robosova <[email protected]>
Co-authored-by: Michaela Robosova <[email protected]>
Co-authored-by: Michaela Robosova <[email protected]>
@Spoorthy1423
Copy link
Contributor Author

Hey!! both tqsm for letting me know we can commit change directly , if anything else needed, can u please let me know, i got some idea on how to make this work - tqsm .

@MisRob
Copy link
Member

MisRob commented Aug 27, 2024

Thanks both here. All looks good to me now. I will merge it now and it will be a part of today's release.

@Spoorthy1423 If part of the reason for so many prs was working with git, this is said to be a fun way to learn more https://learngitbranching.js.org/. Also, I usually recommend authors to preview "Files changed" tab here in GitHub UI to be sure that there are no changes that were not supposed to be there, ideally before marking the PR as ready for review. I really don't know much about you - just my best guesses of what may perhaps help to contribute smoothly.

@MisRob MisRob merged commit 5092490 into learningequality:develop Aug 27, 2024
8 checks passed
@Spoorthy1423
Copy link
Contributor Author

Thank you both for your patience in reviewing my multiple PRs. @MisRob, I will certainly follow your suggestions. The insights and knowledge I gained from this PR are invaluable. Thank you very much.

@MisRob
Copy link
Member

MisRob commented Aug 27, 2024

Thanks for saying that - @AllanOXDi did most of the work (I'm just sticking my nose into things and commenting :) Good luck in your endeavors.

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