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

Resolve PR confilicts #93

Merged
merged 51 commits into from
Dec 14, 2021
Merged

Resolve PR confilicts #93

merged 51 commits into from
Dec 14, 2021

Conversation

jalezi
Copy link
Collaborator

@jalezi jalezi commented Dec 13, 2021

Yesterday @stefanb and I had short discussion about possible PR conflicts. We haven't made any decision.

This is just attempt to solve possible conflicts.

This PR includes: #67, #92, #81, #83

What I did:

  1. from master create new branch "develop"
  2. merge Page not found #67
  3. slowly cherry pick each commit one by one from: Report error #92, 🛂 Add missing react prop types #81 and Fix preserve filter and map state #83.

I’m not sure if it was right decision to include #81 while is still marked as draft, but #83 is based on it.

I skipped #88 while it solves issue #86 only partially and I guess it will be fixed with new design (#89).

Assuming I did everything right, the next step would be prepare PR with new design (#89).

Any thoughts? Especially @mihaerzen, I don't know how far are you with prop-types.

miaerbus and others added 30 commits December 2, 2021 12:55
I had to use react-helmet-async. I was getting error while using
with react-helmet.

I have no idea if this is the right approach.
* CSS(pages)(styles)(Markdown): add span data-term

* Fix: #41 Phone ("tel:" schema) links should not have

target="_blank" attribute

* Fix #52 All external links should open in new tab

* FIX(pages)(Faq): missing target="_blank"

* REFACTOR: use spinner and minor changes

* FIX: partially #57, icon check should be green

* FIX(pages)(Faq): wrong check icon

also remove title from tooltip child

* REFACTOR(comp...)(DoctorCard): minor improvements

* lint fix

* lint fixes

* Fix eslint warnings and errors

Co-authored-by: Stefan Baebler <stefan.baebler@gmail.com>
(cherry picked from commit 4a5f259)
(cherry picked from commit e2f2835)
(cherry picked from commit 8910af2)
jalezi and others added 17 commits December 13, 2021 07:27
change textarea rows as user is typing

(cherry picked from commit 771db12)
... as "accepts" field values in doctors.csv.
See: sledilnik/zdravniki-data

(cherry picked from commit 03ebbcb)
use filterBySearchValueInMapBounds()

(cherry picked from commit 9b4c8a9)
@mihaerzen
Copy link
Collaborator

Hey! The prop types PR is not ready to be merged. I haven't had any time last week to work on it, unfortunately. Also, I would consider it non-blocking so go ahead and deploy without it. The prop-types I would almost consider a nice to have but not crucial for any of the functionalities. Let me know if think different. I can only say I will try to finish it by the end of this week.

@lukarenko
Copy link
Contributor

Quick test report:
✅ page not found look good #67
❓ report error #72 - if we have it on doctor page, maybe we should also have button on search results for faster access to entry form. Should we have another free-form field to add a Note?
✅ preserve search state #83 - looks MVP ready!

In general I think we could merge changes/PRs faster and rebase WIP PR more often to reduce need to resolve conflicts on bigger merges.

@lukarenko
Copy link
Contributor

@jalezi I suspect it is a merge bug that SPREJEMA stays red? Should be green, right?
image

@lukarenko lukarenko mentioned this pull request Dec 13, 2021
before: availability text "SPREJEMA" is red
after: availability text "SPREJEMA" is green

This bug was most likely produced during conflicts resolving.
@jalezi
Copy link
Collaborator Author

jalezi commented Dec 13, 2021

In general I think we could merge changes/PRs faster and rebase WIP PR more often to reduce need to resolve conflicts on bigger merges.

I agree with @lukarenko. Smaller PRs, easier and faster review. I would include your PR @mihaerzen (basically it's one commit). You can slowly fix warnings and maybe even I can give you a hand if I recognize something that is easy to fix (I have plenty of time). Also with each prop-type fix I would add appropriate test, which are desperately needed. As you say prop-types are not essential for functionality and I guess we can take time to fix.

Also I am pretty sure that there is still some useless code that I would like to refactor after we implement new design.

@jalezi
Copy link
Collaborator Author

jalezi commented Dec 13, 2021

@bananica I have also created develop-blue-design branch, where I've (hopefully correct) resolved conflicts with this PR. There are some linting errors ATM (I left them on purpose) in some empty styled components that need your attention.

@stefanb stefanb merged commit dd9d132 into master Dec 14, 2021
@stefanb stefanb deleted the develop branch December 14, 2021 07:06
This was referenced Dec 14, 2021
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.

None yet

6 participants