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 Docs for z-index & drop-shadow for better clarity #819

Conversation

RONAK-AI647
Copy link
Contributor

@RONAK-AI647 RONAK-AI647 commented Nov 8, 2024

Description

The Google Material Design elevation image is not relevant to our approach to drop-shadows, so it should be moved down to the "z-index" section where it is more relevant.

Issue addressed

#813

Addresses #*PR# #819

Before/after screenshots

Changelog

  • Description: This PR addresses the reorganization of content related to Material Design elevation. The Google Material Design elevation image was previously located under the dropdown menu section, which was not relevant to its intended focus on layering and hierarchy. It has been moved to the z-index section for better alignment with layering concepts.
  • Products impact: updated API
  • Addresses: Update Docs for z-index & drop-shadow for better clarity #813
  • Components: no
  • Impacts a11y: no
  • Guidance: Clearer guidance reduces the potential for confusion while referring to the drop-shadow docs.

Steps to test

(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

Comments

The google material design elevation image from dropdown menu is shifted to the z-index section as the image is more relevant there.

@MisRob MisRob requested a review from nucleogenesis November 8, 2024 18:09
Copy link
Member

@nucleogenesis nucleogenesis left a comment

Choose a reason for hiding this comment

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

The documentation changes LGTM - I think that your linting issue might be directly related to the changelog issue -- however, to me your changelog section looks right so I'm wondering if maybe @MisRob might see something I'm missing there.

Thank you @RONAK-AI647 for your work on this!

@MisRob
Copy link
Member

MisRob commented Nov 13, 2024

I'm not sure why the linter check is failing. Sometimes I need to run yarn lint-fix twice. Would you try that @RONAK-AI647 and committed the change? Also it'd be helpful if you could upload your terminal output.

@MisRob
Copy link
Member

MisRob commented Nov 13, 2024

@RONAK-AI647 @nucleogenesis another issue is that this PR contains updated yarn.lock and package.json. That will need to be removed before we can merge. This could help @RONAK-AI647 https://stackoverflow.com/questions/38743912/remove-a-file-from-a-git-pull-request. "Files changed" tab shouldn't contain these two files at all as a result.

@RONAK-AI647
Copy link
Contributor Author

@MisRob please take a look what my terminal output is:

image

@MisRob
Copy link
Member

MisRob commented Nov 14, 2024

Thank you @RONAK-AI647, I don't understand why that's happening. I could see an update made by the linter

Screenshot from 2024-11-14 08-09-28

so I went ahead and pushed the change for you.

@MisRob MisRob requested a review from nucleogenesis November 14, 2024 07:10
@MisRob MisRob added the TODO: needs review Waiting for review label Nov 14, 2024
@RONAK-AI647
Copy link
Contributor Author

Thank you @MisRob for the help , the local server is passing the linter check but it is not passing here in the cicd, i was struggling for it so much.

@MisRob
Copy link
Member

MisRob commented Nov 14, 2024

You're welcome @RONAK-AI647, sorry for the trouble :)

@MisRob
Copy link
Member

MisRob commented Nov 18, 2024

Anything else here @nucleogenesis or could we merge now?

@nucleogenesis nucleogenesis self-assigned this Nov 26, 2024
Copy link
Member

@nucleogenesis nucleogenesis left a comment

Choose a reason for hiding this comment

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

Thanks again for this @RONAK-AI647 !

@nucleogenesis nucleogenesis merged commit c8f037d into learningequality:develop Dec 2, 2024
8 checks passed
learning-equality-bot bot pushed a commit that referenced this pull request Dec 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
TODO: needs review Waiting for review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants