-
Notifications
You must be signed in to change notification settings - Fork 247
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
Improve preview 166 tidy #171
Conversation
I get an error when running
|
I have figured it out its because i was referencing withStyles like
instead of
had to delete lines of code 1by 1 to find that out 🤣will issue a fix shortly |
38afd75
to
5ba5dca
Compare
Ok have rebased a fix for this, build is working for this now |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the amazing work, LGTM 👍
Since this is a Breaking Change, this should be released with the v4.x
version and so I think we should consider carefully how to proceed.
Currently we are maintaining 2 major versions (v3.x
and v2.x
) which have only a minor support difference so we should consider dropping v2.x
and start working toward the complete migration to hooks
(for the v4.x
final release).
Yes that makes total sense, Thanks for reviewing the code, |
Hi @max-carroll , I recently merged some changes into Since
Then I suppose we can start working toward migrating to Also I'm sorry about the changes made to Thanks again for your contribution. |
Hi @panz3r, I'm currently a bit busy with my work at the moment, I'll update you in a week or two if I think I might be able to do some more work on this. If someone wants to fill in for me while I'm absent from contributing that's fine, if I'm in a better position to pitch in within a week or two I'll report back |
Hi @panz3r, I'm going to spend a bit of time on this today |
@panz3r So I will rebase 4.x into my development branch as a starting step and fix conflicts and issues etc? |
Ahh I see what you mean 🤣. Don't worry I have a plan. |
Hi @max-carroll , You should be able to pull v4.x branch and rebase your changes onto that 👍 |
5ba5dca
to
a3ac9c2
Compare
Hi @panz3r . I initially started with a rebase, however as a result of the refactor, it was easier just to do a manual line by line change. Once this has been merged into 4.x, I can begin translation of the code into functional components and implement use of hooks if you like |
@max-carroll can you please change the target branch of this PR to be |
done 😀 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍
Hi @max-carroll , Going to merge this PR. I'll open a new issue and assign that to you to keep track of the migration to Functional components, in the meanwhile can you have a look at PR #173 ? Thanks again for your contribution. |
Opened issue #197 , @max-carroll can you please post a comment so that I can assign that to you, thanks |
Description
Type of change
Props which were previously used to determine the position and behaviour of the preview have changed names and shape: -
*showFileNames => this was previously false by default, however this looks terrible in the new design so we now default to true
How Has This Been Tested
I've tested this on various screensizes ranging from xs-xl on windows chrome,
Ive tested various configs e.g. with custom GetPreviewIcon function, with images, with non images, with mixtures, with varying file limits and up to 20 files
Test Configuration:
windows 10 chrome
Checklist