Skip to content

[FEATURE] Add support for the dvh, lvh and svh length units #415

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

Merged
merged 6 commits into from
Feb 17, 2024

Conversation

KaiOnGitHub
Copy link
Contributor

@KaiOnGitHub KaiOnGitHub commented Aug 1, 2023

Fixes #412

Copy link
Collaborator

@oliverklee oliverklee left a comment

Choose a reason for hiding this comment

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

Hi @KaiOnGitHub, thanks for this PR!

Would you be willing to cover it with a unit test?

@KaiOnGitHub
Copy link
Contributor Author

@oliverklee I fixed the char limit but unfortunately don't have the time to write a test atm. I tested it within our Shopware 6 application and with this change dvh worked while it did not before :)

@oliverklee oliverklee deleted the branch MyIntervals:main February 7, 2024 11:36
@oliverklee oliverklee closed this Feb 7, 2024
@oliverklee oliverklee reopened this Feb 7, 2024
@oliverklee oliverklee changed the base branch from master to main February 7, 2024 22:33
@JakeQZ JakeQZ added unit tests needed PRs with code changes lacking sufficient complementary tests rebase needed enhancement labels Feb 15, 2024
@JakeQZ
Copy link
Collaborator

JakeQZ commented Feb 15, 2024

@KaiOnGitHub, thanks for providing the code changes and testing it within your app. If you still don't have time to add the unit tests, I may be able to pick this up - please don't delete your branch!

@JakeQZ
Copy link
Collaborator

JakeQZ commented Feb 15, 2024

@oliverklee, I tried to rebase this branch, but couldn't figure out what I was supposed to do. This time, I will simply re-apply the changes as a separate PR. But for future reference, what was I doing wrong here?:

d:\git_files\MyIntervals\PHP-CSS-Parser>git pull
remote: Enumerating objects: 1, done.
remote: Counting objects: 100% (1/1), done.
remote: Total 1 (delta 0), reused 1 (delta 0), pack-reused 0
Unpacking objects: 100% (1/1), 521 bytes | 57.00 KiB/s, done.
From https://github.com/MyIntervals/PHP-CSS-Parser
 * [new tag]         v8.5.1     -> v8.5.1
Already up to date.

d:\git_files\MyIntervals\PHP-CSS-Parser>git fetch origin pull/415/head:pr/415
remote: Enumerating objects: 10, done.
remote: Counting objects: 100% (10/10), done.
remote: Compressing objects: 100% (6/6), done.
remote: Total 10 (delta 4), reused 6 (delta 4), pack-reused 0
Unpacking objects: 100% (10/10), 3.81 KiB | 69.00 KiB/s, done.
From https://github.com/MyIntervals/PHP-CSS-Parser
 * [new ref]         refs/pull/415/head -> pr/415

d:\git_files\MyIntervals\PHP-CSS-Parser>git checkout pr/415
Switched to branch 'pr/415'

d:\git_files\MyIntervals\PHP-CSS-Parser>git checkout main
Switched to branch 'main'
Your branch is up to date with 'origin/main'.

d:\git_files\MyIntervals\PHP-CSS-Parser>git pull
Already up to date.

d:\git_files\MyIntervals\PHP-CSS-Parser>git checkout pr/415
Switched to branch 'pr/415'

d:\git_files\MyIntervals\PHP-CSS-Parser>git pull
There is no tracking information for the current branch.
Please specify which branch you want to rebase against.
See git-pull(1) for details.

    git pull <remote> <branch>

If you wish to set tracking information for this branch you can do so with:

    git branch --set-upstream-to=origin/<branch> pr/415


d:\git_files\MyIntervals\PHP-CSS-Parser>git branch --set-upstream-to=origin/main pr/415
branch 'pr/415' set up to track 'origin/main'.

d:\git_files\MyIntervals\PHP-CSS-Parser>git pull
Auto-merging src/Value/Size.php
CONFLICT (content): Merge conflict in src/Value/Size.php
error: could not apply 16b5f18... Add support for dvh, svh, lvh. Close #412
hint: Resolve all conflicts manually, mark them as resolved with
hint: "git add/rm <conflicted_files>", then run "git rebase --continue".
hint: You can instead skip this commit: run "git rebase --skip".
hint: To abort and get back to the state before "git rebase", run "git rebase --abort".
Could not apply 16b5f18... Add support for dvh, svh, lvh. Close #412

d:\git_files\MyIntervals\PHP-CSS-Parser>git checkout main
error: you need to resolve your current index first
src/Value/Size.php: needs merge

d:\git_files\MyIntervals\PHP-CSS-Parser>git add Size.php
fatal: pathspec 'Size.php' did not match any files

d:\git_files\MyIntervals\PHP-CSS-Parser>git add *.php

d:\git_files\MyIntervals\PHP-CSS-Parser>git rebase --continue
[detached HEAD 7039532] Add support for dvh, svh, lvh. Close #412
 Author: Kai König <50620424+KaiOnGitHub@users.noreply.github.com>
 1 file changed, 17 insertions(+), 1 deletion(-)
Auto-merging src/Value/Size.php
CONFLICT (content): Merge conflict in src/Value/Size.php
error: could not apply bc058a4... fix 120 char per line limit
hint: Resolve all conflicts manually, mark them as resolved with
hint: "git add/rm <conflicted_files>", then run "git rebase --continue".
hint: You can instead skip this commit: run "git rebase --skip".
hint: To abort and get back to the state before "git rebase", run "git rebase --abort".
Could not apply bc058a4... fix 120 char per line limit

d:\git_files\MyIntervals\PHP-CSS-Parser>git push -f
fatal: You are not currently on a branch.
To push the history leading to the current (detached HEAD)
state now, use

    git push origin HEAD:<name-of-remote-branch>


d:\git_files\MyIntervals\PHP-CSS-Parser>git push origin HEAD:pr/415
error: The destination you provided is not a full refname (i.e.,
starting with "refs/"). We tried to guess what you meant by:

- Looking for a ref that matches 'pr/415' on the remote side.
- Checking if the <src> being pushed ('HEAD')
  is a ref in "refs/{heads,tags}/". If so we add a corresponding
  refs/{heads,tags}/ prefix on the remote side.

Neither worked, so we gave up. You must fully qualify the ref.
hint: The <src> part of the refspec is a commit object.
hint: Did you mean to create a new branch by pushing to
hint: 'HEAD:refs/heads/pr/415'?
error: failed to push some refs to 'https://github.com/MyIntervals/PHP-CSS-Parser.git'

d:\git_files\MyIntervals\PHP-CSS-Parser>git push origin HEAD:refs/pull/415/head
Enumerating objects: 9, done.
Counting objects: 100% (9/9), done.
Delta compression using up to 8 threads
Compressing objects: 100% (5/5), done.
Writing objects: 100% (5/5), 576 bytes | 576.00 KiB/s, done.
Total 5 (delta 4), reused 0 (delta 0), pack-reused 0
remote: Resolving deltas: 100% (4/4), completed with 4 local objects.
To https://github.com/MyIntervals/PHP-CSS-Parser.git
 ! [remote rejected] HEAD -> refs/pull/415/head (deny updating a hidden ref)
error: failed to push some refs to 'https://github.com/MyIntervals/PHP-CSS-Parser.git'

d:\git_files\MyIntervals\PHP-CSS-Parser>git add *.php

d:\git_files\MyIntervals\PHP-CSS-Parser>git rebase --continue
Successfully rebased and updated refs/heads/pr/415.

d:\git_files\MyIntervals\PHP-CSS-Parser>git push -f
fatal: The upstream branch of your current branch does not match
the name of your current branch.  To push to the upstream branch
on the remote, use

    git push origin HEAD:main

To push to the branch of the same name on the remote, use

    git push origin HEAD

To choose either option permanently, see push.default in 'git help config'.

To avoid automatically configuring an upstream branch when its name
won't match the local branch, see option 'simple' of branch.autoSetupMerge
in 'git help config'.


d:\git_files\MyIntervals\PHP-CSS-Parser>git push origin HEAD
Enumerating objects: 9, done.
Counting objects: 100% (9/9), done.
Delta compression using up to 8 threads
Compressing objects: 100% (5/5), done.
Writing objects: 100% (5/5), 576 bytes | 576.00 KiB/s, done.
Total 5 (delta 4), reused 0 (delta 0), pack-reused 0
remote: Resolving deltas: 100% (4/4), completed with 4 local objects.
remote:
remote: Create a pull request for 'pr/415' on GitHub by visiting:
remote:      https://github.com/MyIntervals/PHP-CSS-Parser/pull/new/pr/415
remote:
To https://github.com/MyIntervals/PHP-CSS-Parser.git
 * [new branch]      HEAD -> pr/415

@JakeQZ
Copy link
Collaborator

JakeQZ commented Feb 15, 2024

Step 1: From your project repository, check out a new branch and test the changes.

git checkout -b KaiOnGitHub-patch-1 main
git pull https://github.com/KaiOnGitHub/PHP-CSS-Parser.git patch-1

Step 2: Merge the changes and update on GitHub.

git checkout main
git merge --no-ff KaiOnGitHub-patch-1
git push origin main

Trying this now.

@JakeQZ
Copy link
Collaborator

JakeQZ commented Feb 15, 2024

Step 1: From your project repository, check out a new branch and test the changes.

git checkout -b KaiOnGitHub-patch-1 main
git pull https://github.com/KaiOnGitHub/PHP-CSS-Parser.git patch-1

Step 2: Merge the changes and update on GitHub.

git checkout main
git merge --no-ff KaiOnGitHub-patch-1
git push origin main

Trying this now.

I can see this is totally stupid. It will clearly merge the changes into main without review. Fortunately I already know that the suggestions from git and GitHub are usually utter rubbish, so have not done this.

Much simpler to just start over. Avoids a waste of time with systems that don't work or cannot be understood.

@oliverklee
Copy link
Collaborator

@JakeQZ Maybe the GitHub documentation for working on a PR branch of a fork is helpful: https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/proposing-changes-to-your-work-with-pull-requests/committing-changes-to-a-pull-request-branch-created-from-a-fork

(I have to admit that I have never tries this myself.)

@sabberworm
Copy link
Collaborator

@JakeQZ Maybe the GitHub documentation for working on a PR branch of a fork is helpful

Yes, if the PR submitter has enabled “allow contributions from maintainers” setting, you’ll be able to commit to their forked branch.

However, I find that, for simple merge conflicts, the web-based UI GitHub provides is usually sufficient (I used it in this case).

For now, the `TestCase` just tests that all the unit values are parsed.
(Other tests can be added here, but are beyond the scope of this change.)
@JakeQZ
Copy link
Collaborator

JakeQZ commented Feb 17, 2024

Yes, if the PR submitter has enabled “allow contributions from maintainers” setting, you’ll be able to commit to their forked branch.

I think I maybe went wrong by fetching the remote PR into my local repo. Didn't have a problem after cloning their fork into a separate directory.

@JakeQZ JakeQZ removed unit tests needed PRs with code changes lacking sufficient complementary tests rebase needed labels Feb 17, 2024
Co-authored-by: Oliver Klee <github@oliverklee.de>
Co-authored-by: Oliver Klee <github@oliverklee.de>
@JakeQZ JakeQZ requested a review from oliverklee February 17, 2024 13:14
@oliverklee oliverklee changed the title Add support for dvh, svh, lvh. Close #412 [FEATURE] Add support for dvh, svh, lvh Feb 17, 2024
@oliverklee oliverklee changed the title [FEATURE] Add support for dvh, svh, lvh [FEATURE] Add support for the dvh, lvh and svh length units Feb 17, 2024
@oliverklee oliverklee merged commit cb78396 into MyIntervals:main Feb 17, 2024
JakeQZ pushed a commit that referenced this pull request Jun 28, 2024
Fixes #412

For now, the `TestCase` just tests that all the unit values are parsed.
(Other tests can be added here, but are beyond the scope of this change.)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add support for new viewport unit in Size class
4 participants