Update dropshadows to the latest guidelines #736
Conversation
replaced redundant dropshadows with most relavent version out of dropshadow(1,2,6)
switched out the box shadow with appropriate drop-shadows
added the import functions to the files missing them
|
Thanks @lokesh-sagi125. Did very brief skim and overall it seems to me it does what it's supposed to :) I will invite my colleagues for a full review. Meanwhile, please run |
|
yes @MisRob on it |
|
hey , just ran yarn-fix and updated the change log, sorry for the inconvenience i am new to contributing |
|
@lokesh-sagi125 Sure, no problem at all, we're happy to provide guidance to new contributors :) Thanks! |
|
@lokesh-sagi125 would you be able to resolve the conflict? I think you updated the styling page but I also updated it as part of the writing styling guidance that you followed. I just merged my PR to Thanks for taking care of the file too and sorry I have forgotten to mention this when you picked up the issue. |
|
@MisRob done with including the changes, can you try merging again? |
|
I already merged my work @lokesh-sagi125, that's why you saw the conflict. But I see you resolved that now! Looking good, thanks. |
|
hey @MisRob is there sth wrong with the pr or can i move on to another issue? |
|
Hi @lokesh-sagi125, I don't think that anything is wrong :) Seems nobody got to do the review yet. You don't need to wait for a review to be finished to work on another task, so you're welcome to pick something. |
There was a problem hiding this comment.
Hi @lokesh-sagi125! great work on the clean up. As opposed to commenting out the old code, It makes more sense to remove it altogether, as we wont need it :). Based on the acceptance criteria,
However, the unused %dropshadow-...s shouldn’t be deleted from definitions.scss yet (they are still being referenced from some non-KDS components in the products).
we can safely delete them save for the aforementioned exception.
CHANGELOG.md
Outdated
There was a problem hiding this comment.
Leaving a note here that before merging it'd be good to revert CHANGELOG.md changes since the new GH action will now update the CHANGELOG.md after merging this PR by pasting the changelog from the PR description. Otherwise we'd have duplicate entries.
|
Thanks for following-up @lokesh-sagi125. cc @akolson will leave the final approve on you, just leaving a note that I already updated the changelog description in the PR. |
yarn.lock
Outdated
There was a problem hiding this comment.
Would you revert this file @lokesh-sagi125?
There was a problem hiding this comment.
Ideally this file shouldn't be a part of the pr. Could you please do this and the revert should be complete. Thanks again @lokesh-sagi125
There was a problem hiding this comment.
Hi @lokesh-sagi125 apologies on the back and forth, but this latest change would now cause breakages as yargs-parser... dependency has been removed. Could you please do this as suggested by my colleague @MisRob
There was a problem hiding this comment.
After the revert of the yarn.lock, we should be good with this. Leaving a blocker here to prevent accidental merge. But all is good. Thanks @lokesh-sagi125 for your contribution!
yarn.lock
Outdated
| @@ -15372,4 +15372,4 @@ yargs@^16.2.0: | |||
| require-directory "^2.1.1" | |||
| string-width "^4.2.0" | |||
| y18n "^5.0.5" | |||
| yargs-parser "^20.2.2" | |||
| yargs-parser "^20.2.2" | |||
There was a problem hiding this comment.
Please remove this added new line that is causing the diff in this file.
|
Hi @lokesh-sagi125, another way to formulate what we're trying to say with @akolson regarding This is a nice technique for removing file from a pull request https://stackoverflow.com/a/46571620 |
|
the yarn.lock is not getting exclude through git commands , let me re-open the pr , sorry for the inconvenience |
|
@lokesh-sagi125 no need to go through the trouble, we'll fix this on our side. Your pull request almost ready for the merge. Re-opening and reverting the yarn.lock. Thanks Again! |
|
@lokesh-sagi125 I have already made the revert and will be pushing shortly. :) |
|
No problem @lokesh-sagi125, it was just 1 file out of 13 ;) However I'm sorry to say you've just closed it by deleting your fork, and we can't re-open in this case. |
|
oh im really sorry about that ; i was on the wrong user let me reopen it |
|
@lokesh-sagi125 I am pretty sure that was accidental. Just let us know if you wish to open a new PR ;) |
|
No worries. |
|
@lokesh-sagi125 in case it's much of trouble, please let me know as I can create a branch with your changes if necessary. Thanks |
Description
updating dropshadow according to the latest guidelines and replacing box-shadow eith appropriate dropshadow
updated the elemnts in the kds repo to use the dropshadows according to the latest guidelines and replace other elements like boxshadows with appropriate dropshadow.
Addresses #724
Changelog