Skip to content

Conversation

@bl4ckm0r3
Copy link
Contributor

I have kept working on the repo so now it should feature better position handling (px instead of %, there could be some refactor possible but haven't had time yet).
Also I have swapped the inline-styles (that make it a nightmare to override) to use classes.

Marco Sparagna and others added 20 commits October 27, 2017 17:50
Adding individual region s tyling through data
TODO refactor use of refs
Upgraded packages and small refactors
New release, fixed peerdependency
removed regionStyle
@d0b1010r
Copy link
Contributor

d0b1010r commented Dec 7, 2017

Wow. Many changes - that makes it hard to have an overview what actually is in here :)

  1. inline styles vs css classes - Yes that makes definitely sense - but at the same time, if you use it as it currently is, then you don't need to include any css for this module. But all in all it's not a big thing. Bad thing is I just removed scss support from repo :D - but can add it again. This I like 👍

  2. Zoom feature: Whats this about?

  3. Why the change from percentage to pixel? We use percentage because that makes it easy to translate the same data on our backend and front end.

  4. You also inlined the image src – this we actually can not merge, because we display two images above each other. What's the reasoning behind this?

@bl4ckm0r3
Copy link
Contributor Author

Hehe yesterday I didn't have much time to write a proper changelog.

  1. I understand the advantage of not embedding sass, but libraries shouldn't bind you over a style.
    I was thinking about something like emotion https://github.com/emotion-js/emotion that sits in between.
  2. you can zoom into images and leave annotation that would scale and you can scroll in the document now
  3. it's easier to reason about and it made 2) a lot easier to develop. You can still save it in percentage in your backend since you have the height and width of the document.
  4. ohhh i was wondering why, had some issues with the offset but i can revert that one.
    ok you can close this pr and i'll make a new one when things are ready.

@d0b1010r
Copy link
Contributor

d0b1010r commented Dec 7, 2017

  1. Yes that also sounds good - although I never used any css-in-js module :) – and I couldn't find any information if and how using emotion for packages.

  2. Sounds interesting and useful and we can change our usage (3) accordingly.

  3. -> yes a new updated PR would be good! Can you also rebase against the current master? Then the merge can be done without any conflicts.

bl4ckm0r3 and others added 3 commits December 7, 2017 11:30
refactored to allow multiple images
added multiple image feature
code refactor
@d0b1010r
Copy link
Contributor

d0b1010r commented Dec 8, 2017

Can I close?

@d0b1010r
Copy link
Contributor

d0b1010r commented Dec 8, 2017

I'll just do :) In favor if #11

@d0b1010r d0b1010r closed this Dec 8, 2017
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