Skip to content

Conversation

@znegrin
Copy link
Contributor

@znegrin znegrin commented Jan 24, 2019

Here are the updated contents of the UI Pattern Library to include the new block of categories and cards to show source{d} Engine snippets on the Product page.

It is related to the document Landing Quick fixes & improvements and the issue #1340 of the backlog.

image

Signed-off-by: znegrin zurinegrin@gmail.com

Signed-off-by: znegrin <zurinegrin@gmail.com>
@ricardobaeta ricardobaeta self-requested a review January 25, 2019 13:08
Copy link
Contributor

@ricardobaeta ricardobaeta left a comment

Choose a reason for hiding this comment

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

Hi @znegrin Looking good! Just please be so kind as to add the carousel tabs and cards as UI library symbols also, inside the component you've just added. And let's wait for us to be able to see these components in the browser before we merge this.

Last, It seems you have some conflicts. Did you pull the repo before making the PR?

Thanks!

@znegrin
Copy link
Contributor Author

znegrin commented Jan 25, 2019

Hi @ricardobaeta

For sure I did the pull, obviously, and of course I added the blocks to the library. I add here a screenshot:

image

Maybe it is something related to the conflict... Maybe it is happening because this is a file format that git cannot recognize and merge properly?

@ricardobaeta
Copy link
Contributor

Hi @znegrin ! The entire block yes, but not the tab navigation element, neither the card itself. It can be useful in the future :)

You did a PR already, and I've merged it, with ease. There's must be something wrong with it. In alternative you can pull from the repo, and add the element again, it's already designed. Just please make sure you can handle this conflict.

Thanks!

Signed-off-by: znegrin <zurinegrin@gmail.com>
@znegrin
Copy link
Contributor Author

znegrin commented Jan 25, 2019

Hi @ricardobaeta,

I just added a commit with the additions that you requested and also trying yo fix the conflicts.

image

Please let me know if it is fine now.

Thanks

@ricardobaeta
Copy link
Contributor

ricardobaeta commented Jan 25, 2019

Hi @znegrin Looking good! Make sure you pull from origin. Thanks!

backup first

@znegrin
Copy link
Contributor Author

znegrin commented Jan 25, 2019

Hi @ricardobaeta

I always pull from origin. I have no idea that you can pull in any other way, so maybe the problem is another thing.

image

@ricardobaeta
Copy link
Contributor

You should update your fork - .../znegrin/design.git - pulling from origin - .../src-d/design - if I'm not mistaken.

@znegrin
Copy link
Contributor Author

znegrin commented Jan 25, 2019

I update my fork every time I pull from master, and from what I see, it is updated:

image

@marnovo
Copy link
Member

marnovo commented Jan 25, 2019

@znegrin keep in mind this PR was originally wants to merge 2 commits into src-d:master from znegrin:product-page-update, so maybe the issue is comparing a different branch as product-page-update (not master) from your fork?

@znegrin
Copy link
Contributor Author

znegrin commented Jan 25, 2019

Hi @marnovo

Sorry I really don't understand. I updated master from origin, and then from there I updated the product-page-update branch with the requested changes, which is the branch that needs to be compare right?

@znegrin
Copy link
Contributor Author

znegrin commented Feb 5, 2019

@ricardobaeta @marnovo it seems that the problem is not a "real" conflict, but as I suggested, there is a problem between Git and binary files that always get a conflict like this one. I found some information about that:

https://codeyarns.com/2016/04/11/how-to-resolve-git-conflict-with-binary-files/
https://www.designernews.co/stories/73398-does-sketch-work-well-with-git-merge-

I also found what seems to be a tool that can be helpful to use Git with .sketch files:
https://kactus.io/

It come from this: https://github.com/mathieudutour/git-sketch-plugin

Maybe it is something we could try. For this one I suggest to just accept it after opening and reviewing the files. What do you think?

@dpordomingo
Copy link
Contributor

dpordomingo commented Feb 6, 2019

@ricardobaeta asked me for some insights into what's happening here:

$ git log
*  7639c1e 25-ene-2019 18:45 znegrin (HEAD -> PR17-product-page-update) 
|  - Fixing merge and adding individual elements
| 
*  2997803  24-ene-2019 18:05 znegrin
|  - Adding new Product section with cards and tabs
| 
|  *   17f2383  21-ene-2019 18:43 Ricardo Baeta (origin/master) 
|  |\  - Merge pull request #16 from znegrin/engine-modules
| / /  
|/ /  
| *  65f6294  21-ene-2019 19:31 Zuri Negrín
| |  - Adding the new layout for the sub-section advantages to the pattern library
|/  
|   
*  48c9aa6  21-ene-2019 12:31 Marcelo Novaes (old-master) 
|  - Merge pull request #15 from src-d/fix-theme-link
|

As @marnovo pointed it out (#17 (comment)), the branch being pullrequested product-page-update is modifying files that were already modified in src-d:master:

This PR#17 modifies:

$ git diff old-master..PR17-product-page-update
- ui-pattern-library/files/landing-pattern-library.sketch
- ui-pattern-library/files/typography.sketch
- ui-pattern-library/files/ui-elements.sketch

but it is not aware of changes already done in the previous PR #16 over the same files:

$ git diff old-master..origin/master
- ui-pattern-library/files/landing-pattern-library.sketch
- ui-pattern-library/files/typography.sketch
- ui-pattern-library/files/ui-elements.sketch

Since this PR is trying to modify files that were already modified in src-d:master, git forces you to decide what to do:

  • discard the other changes,
  • discard these changes,
  • merge them manually: applying the current changes over the files in src-d:master.

What it is said at https://codeyarns.com/2016/04/11/how-to-resolve-git-conflict-with-binary-files/ (as @znegrin linked), is only considering the first two options, but it will imply that the work of one or the other side will be lost.

In most cases, both work —from both sides— needs to be kept, so it is mandatory to manually apply the changes done in the other side before merging.

Only when you know that the changes in your branch already contains the ones in the another, you can "ignore" the others.

Was this explanation enough? Can I help you in any other way?

@dpordomingo
Copy link
Contributor

dpordomingo commented Feb 6, 2019

addendum
☝️ this problem happens a lot with binnary files, because Git can not know if the changes in one side were already considered in the other side.

To avoid facing this problem once and again and again, people use to avoid modifying in parallel (in two different branches) the very same file (because the branch being merged first will win, and the second branch will face the problem).

@znegrin
Copy link
Contributor Author

znegrin commented Feb 6, 2019

Thank you for the information @dpordomingo. :)

I think the version in this PR is the most updated one right now, but it is only my guess, as I added to the files all the new and missing blocks, so as I said before, I think we can keep this one because it has all the new modules.

What do you think @ricardobaeta @marnovo ?

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.

4 participants