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

Update dropshadows to the latest guidelines #724

Closed
2 tasks
MisRob opened this issue Aug 11, 2024 · 17 comments
Closed
2 tasks

Update dropshadows to the latest guidelines #724

MisRob opened this issue Aug 11, 2024 · 17 comments
Assignees
Labels
DEV: frontend good first issue Self-contained, straightforward, low-complexity help wanted Open source contributors welcome P2 - normal Priority: Nice to have

Comments

@MisRob
Copy link
Member

MisRob commented Aug 11, 2024

🌱 Are you new to the codebase? Welcome! Please see the contributing guidelines.

Blocks

Related

Summary

Locate all KDS components that use dropshadows and update their style to follow the updated guidelines in the "Elevation and shadow > Drop shadows" documentation section.

For example, this KModal ’s dropshadow

.modal {
@extend %dropshadow-16dp;

needs to be changed to %dropshadow-6dp.

Note that some components have dropshadows that are not defined via %dropshadow-... , for example:

box-shadow: 0 1px 3px rgba(0, 0, 0, 0.4);

These need to be replaced by appropriate %dropshadow-... as well.

Guidance

  • See
    // Use this mixin in select containers where the content is likely going to be
    for all dropshadows to search for. However, use only %dropshadow-1dp, %dropshadow-2dp, and %dropshadow-6dp for all updated places. The remaining ones will be removed in accordance with the new guidelines later on.
  • Updating forked components, such as Ui components (lib/keen/) or Popper.vue, is desired (we don’t upgrade to their upstream latest versions anymore)
  • Preview components as you work. You can preview many components on their documentation pages when running the development server. If there’s a live example missing, you can use the playground page as described in the "Preview updates in the Kolibri Design System environment"

Acceptance Criteria

  • There are no occurrences of %dropshadow-… usage other than 1dp, 2dp, and 6dp in the whole KDS codebase, nor are there any dropshadow styles defined in ways different from %dropshadow-....
  • 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).
@lokesh-sagi125
Copy link
Contributor

hi, i see that no one is assigned to this issue can i work on this ?

@MisRob
Copy link
Member Author

MisRob commented Aug 17, 2024

Yes @lokesh-sagi125, thanks. I will assign you. Please familiarize yourself at first with the development server, the documentation site, and the playground page - all is linked in the contributing guidelines. Try to follow the issue guidance closely and message me if you had any questions. I also provided some additional guidance to a contributor who is currently working on the same issue but in a different repository, so you can read through it learningequality/kolibri#12552.

@lokesh-sagi125
Copy link
Contributor

lokesh-sagi125 commented Aug 17, 2024

hi, which drop shadow should i use for hover and focus is it dropshadow-6dp?

@MisRob
Copy link
Member Author

MisRob commented Aug 17, 2024

@lokesh-sagi125 Yes, 6dp

@MisRob
Copy link
Member Author

MisRob commented Aug 17, 2024

@lokesh-sagi125 I don't think focus is explicitly mentioned, so thanks for raising that! Just to be sure you're aware of this guidance for other situations, there are some examples in the linked "Elevation and shadow > Drop shadows". If there's something else missing, you're welcome reach out again here.

@lokesh-sagi125
Copy link
Contributor

hello @MisRob , i have finished replacing all the elements that use box-shadow and repricated dropshadows with appropriate
replacements, but some elements like -moz-box-shadow: rgb(58, 58, 58) 0 0 6px 0; seem to be using colors in their shadow what should i do for these?

@MisRob
Copy link
Member Author

MisRob commented Aug 19, 2024

Thanks @lokesh-sagi125! Ah I see, let me check on this and see if I need to talk our designers. Would you have a moment to post a screenshot of the shadow you're referring to so I can preview?

@lokesh-sagi125
Copy link
Contributor

sorry @MisRob ,but were you asking for the code or the photo of the component?

@MisRob
Copy link
Member Author

MisRob commented Aug 19, 2024

Photo

@lokesh-sagi125
Copy link
Contributor

@MisRob could you please check my draft pr once, i think ive made all the necessary changes requires

@MisRob
Copy link
Member Author

MisRob commented Aug 19, 2024

Hi @lokesh-sagi125, yes thank you! I will follow-up some time this week.

@lokesh-sagi125
Copy link
Contributor

i also resolved the issue #725 will PR once the other blocking issue is solved

@AlexVelezLl
Copy link
Member

Hey @lokesh-sagi125, could you add a comment in that issue asking to be assigned? So we can assign this to you and avoid it getting assigned to anyone else.

@lokesh-sagi125
Copy link
Contributor

hey @MisRob i am getting an error "changelog.md is not changes" i am new to contributing, can you tell me what i am supposed to do?

@AlexVelezLl
Copy link
Member

Hi @lokesh-sagi125! This is because we need to create new entries in our Changelog.md file for any changes we do in KDS so we can effectevily communicate this in our next release.

When you opened your PR, you should have seen a template. You should not delete that template, but rather fill in the details there. And one of those sections is precisely "Changelog" which is where the details are written that will later be pasted to the Changelog.md file. For now you can ignore that check, once the code review has passed you can paste the content of this Changelog section into the changelog.md file.

For now to recover the template, you can copy the content of this file and fill in the details :).

@MisRob
Copy link
Member Author

MisRob commented Aug 21, 2024

Thanks @AlexVelezLl and @lokesh-sagi125 :-)

@lokesh-sagi125 we will follow-up in the PR later on, thanks!

@akolson
Copy link
Member

akolson commented Aug 30, 2024

Fixed with #762

@akolson akolson closed this as completed Aug 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
DEV: frontend good first issue Self-contained, straightforward, low-complexity help wanted Open source contributors welcome P2 - normal Priority: Nice to have
Projects
None yet
Development

No branches or pull requests

5 participants