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

Inset shadows #393

Closed
wants to merge 5 commits into from
Closed

Inset shadows #393

wants to merge 5 commits into from

Conversation

Vidhanvyrs
Copy link

changes done on props.shadows.js according to the codepen example that was given with the issue
Please provide feedback and point out if any particular changes required

@stackblitz
Copy link

stackblitz bot commented Aug 18, 2023

Review PR in StackBlitz Codeflow Run & review this pull request in StackBlitz Codeflow.

Copy link
Owner

@argyleink argyleink left a comment

Choose a reason for hiding this comment

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

thanks for working on this! ❤️

can you reduce the PR changes to just the code added please? looks like a formatter ran and is making it hard to decipher what you added.

can you change the shadow additions to only be the bottom inset color, like this inset 0 -.5px 0 0 #fff1?

remove the modifications to --inner-shadow-0 it doesnt need the trick

run npm run lib:shadows to generate the css file from the changes to this js file.

After these changes, I'll create a few tester examples to make sure the trick isnt disrupting anything and is in fact an improvement. Thanks again! no one will probably notice this change, but one day, they'll see its just slightly more realistic 😁

@Vidhanvyrs
Copy link
Author

Thankyou @argyleink
The required changes will be done in no time
I'M ON IT!

@Vidhanvyrs
Copy link
Author

Extremely sorry i accidently added the commit without running the npm run lib: shadows in the commit 7e9aa4c
but i do performed it in commit 0d1ba17
So now i am clarifying about the changes! Please forgive 🙇‍♂️

src/props.shadows.js Show resolved Hide resolved
'--inner-shadow-3': 'inset 0 2px 8px 0 hsl(var(--shadow-color) / calc(var(--shadow-strength) + 9%))',
'--inner-shadow-4': 'inset 0 2px 14px 0 hsl(var(--shadow-color) / calc(var(--shadow-strength) + 9%))',
}
"--inner-shadow-0":
Copy link
Owner

Choose a reason for hiding this comment

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

inner-shadow-0 shouldnt have any changes 🙂

Comment on lines 46 to 52
inset 0 -.5px 0 0 #fff1`,
"--inner-shadow-2": `
inset 0 -.5px 0 0 #fff1`,
"--inner-shadow-3": `
inset 0 -.5px 0 0 #fff1`,
"--inner-shadow-4": `
inset 0 -.5px 0 0 #fff1`,
Copy link
Owner

Choose a reason for hiding this comment

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

oh no, looks like the inner shadows are gone and now there's only the highlight. typo maybe?!

Copy link
Author

Choose a reason for hiding this comment

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

yeah that is a typo sorry correcting it

Comment on lines +59 to +71
.map(([key, value]) => {
value = value.replace(
/var\(--shadow-color\)/g,
Shadows["--shadow-color"]
);
value = value.replace(
/var\(--shadow-strength\)/g,
Shadows["--shadow-strength"]
);
return [key, value];
})
.filter(([key]) =>
!['--shadow-color', '--shadow-strength'].includes(key)
)
)
.filter(([key]) => !["--shadow-color", "--shadow-strength"].includes(key))
);
Copy link
Owner

Choose a reason for hiding this comment

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

formatter changes again? this should be unchanged

Copy link
Author

Choose a reason for hiding this comment

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

how can i do this no formatter interference because before committing, the code is the same before but when i committed its the formatter that is doing this
Can you please suggest me how can i resolve this formatter thing this is what chatgpt suggested

If you want to prevent your code formatter from automatically formatting your code upon committing, you can take a few steps depending on the tools you're using. Here's how you can do it using Git and popular code formatters like Prettier:

Git Hooks: You can use Git hooks to run scripts before or after certain Git events, such as committing. You can create a pre-commit hook that temporarily disables the formatter before committing. Here's a general idea:

Create a .pre-commit-config.yaml file in your project's root directory (if it doesn't already exist).

Configure the hook to disable the formatter for the staged files using the formatter's command-line options. For example, if you're using Prettier, you can run:

yaml
Copy code

  • repo: https://github.com/pre-commit/mirrors-prettier
    rev: v2.5.1
    hooks:
    • id: prettier
      args: ["--write", "$PRE_COMMIT_HOME/../$PRE_COMMIT_FILES"]
      This configuration will use Prettier to format the files, but it will only format the staged files as specified by $PRE_COMMIT_FILES.

Commit the .pre-commit-config.yaml file along with your code.

Configuration Files: Some code formatters, like Prettier, allow you to configure their behavior through configuration files like .prettierrc or prettier.config.js. You can include these configuration files in your project and customize the formatting options to match your preferred style. This way, the formatter will consistently apply the same formatting rules.

Remember, while these methods can help you maintain a consistent code style, it's also a good practice to collaborate with your team and agree on a shared code formatting style that everyone follows. This can help reduce conflicts related to formatting in pull requests.

Copy link
Owner

Choose a reason for hiding this comment

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

i think you need to disable your formatter in this project from running when you press save. it's a common feature people have when they work on teams and they've agreed on that formatter, but this project is different and your formatter is trying to do the right thing based on it's settings, but it's also making your PR change more than it needs to. i dont know which formatter you have installed on your machine, so i can't give particular instructions. but you could try disabling it entirely while editing this file and then enabling it again once back on your team?

sorry this sounds tedious! but it's also good practice to make the smallest change needed for the feature. changing a whole files single quotes to double quotes, is more than what's needed for the feature. thanks for working some of those formatter changes back, sorry they're in your way.

i'm also on vacation =) will check back here in a couple days

@Vidhanvyrs
Copy link
Author

yeah so @argyleink now i think the commit 157808f is correct there is no formatter disturbance in this commit and the shadow is also going well
waiting for the review

Comment on lines +2 to +10
"--shadow-color": "220 3% 15%",
"--shadow-strength": "1%",

'--shadow-color-@media:dark': '220 40% 2%',
'--shadow-strength-@media:dark': '25%',

'--shadow-1': '0 1px 2px -1px hsl(var(--shadow-color) / calc(var(--shadow-strength) + 9%))',
'--shadow-2': `
"--shadow-color-@media:dark": "220 40% 2%",
"--shadow-strength-@media:dark": "25%",

"--shadow-1":
"0 1px 2px -1px hsl(var(--shadow-color) / calc(var(--shadow-strength) + 9%))",
"--shadow-2": `
Copy link
Owner

Choose a reason for hiding this comment

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

these changes shouldn't be part of the PR 😅

Comment on lines +43 to +52
"inset 0 0 0 1px hsl(var(--shadow-color) / calc(var(--shadow-strength) + 9%))",
"--inner-shadow-1": `
inset 0 -.5px 0 0 hsl(var(--shadow-color) / calc(var(--shadow-strength) + 9%))`,
"--inner-shadow-2": `
inset 0 -.5px 0 0 hsl(var(--shadow-color) / calc(var(--shadow-strength) + 9%))`,
"--inner-shadow-3": `
inset 0 -.5px 0 0 hsl(var(--shadow-color) / calc(var(--shadow-strength) + 9%))`,
"--inner-shadow-4": `
inset 0 -.5px 0 0 hsl(var(--shadow-color) / calc(var(--shadow-strength) + 9%))`,
};
Copy link
Owner

Choose a reason for hiding this comment

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

looks like another typo here because the inner shadows are now only the highlight and don't have the inner shadow part

Screenshot 2023-08-29 at 3 41 37 PM

Copy link
Owner

Choose a reason for hiding this comment

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

just to be clear too, this PR should really only be changing these lines. need to combine the highlights like you added above, with the previous shadows that were there. thinking about it too, the highlight could be another custom property, which would help us not repeat the same highlight shadow over and over.

@argyleink
Copy link
Owner

argyleink commented Aug 29, 2023

here! i wont merge this PR i'm linking because i want you to get contribution credit for all the work you've put in so far, but this is the PR shape I'm lookin for 🙂

#399

@argyleink
Copy link
Owner

hi @Vidhanvyrs, you still interested in contributing this? #399 is ready to go, shows all the changes needing to be made, but I'd still totally prefer if you got credit for the work you've done so far so I don't want to merge it 🙂

@Vidhanvyrs
Copy link
Author

No thankyou its fine you lot can continue BTW thanx!!🙇‍♂️

@argyleink argyleink closed this Sep 20, 2023
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.

2 participants