-
Notifications
You must be signed in to change notification settings - Fork 10
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
NEW: CMS 5 upgrade. #102
NEW: CMS 5 upgrade. #102
Conversation
6c41d99
to
3925450
Compare
fd9f6fb
to
9ab8d14
Compare
I've taken a stab at updating the javascript dependencies. Seems to work as expected locally, but I have very limited exposure to this module before now so please do a thorough regression test. Note that I have disabled jest tests on |
import { Tooltip } from 'reactstrap'; | ||
import { UncontrolledTooltip as Tooltip } from 'reactstrap'; |
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.
The state control we were doing here was pointless - it was just toggling a boolean in state. No need for us to manage that directly.
const revertButtonTitle = isReverting | ||
? i18n._t('HistoryViewerToolbar.REVERT_IN_PROGRESS', 'Revert in progress...') | ||
: i18n._t('HistoryViewerToolbar.REVERT_UNAVAILABLE', 'Unavailable for the current version'); | ||
let revertButtonTitle = ''; | ||
if (isReverting) { | ||
revertButtonTitle = i18n._t('HistoryViewerToolbar.REVERT_IN_PROGRESS', 'Revert in progress...'); | ||
} else if (isLatestVersion) { | ||
revertButtonTitle = i18n._t('HistoryViewerToolbar.REVERT_UNAVAILABLE', 'Unavailable for the current version'); | ||
} |
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.
Without this, we end up with two tooltips when hovering over the revert button for the history details of a related child record.
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.
We can't use hooks in a class component, so we need this little HOC to handle the hook for us.
We can't use the old version of the resize aware library because that doesn't support the new react dependency constraint.
react-resize-aware@^3.1.2: | ||
version "3.1.2" | ||
resolved "https://registry.yarnpkg.com/react-resize-aware/-/react-resize-aware-3.1.2.tgz#a5e6fe4691a3ac8d3b0fadd37008339db2294c4f" | ||
integrity sha512-sBtMIEy/9oI+Xf2o7IdWdkTokpZSPo9TWn60gqWKPG3BXg44Rg3FCIMiIjmgvRUF4eQptw6pqYTUhYwkeVSxXA== | ||
react-resize-aware@^4.0.0: | ||
version "4.0.0" | ||
resolved "https://registry.yarnpkg.com/react-resize-aware/-/react-resize-aware-4.0.0.tgz#9a2e002c0a3d15285ccd81c2d3d17aff64e8a597" | ||
integrity sha512-42aoj3uGcqhsMQE7yssxuy0+YYNp38CSzoGxRXUV80WUm6uYJKhAwBlOSBltxTbdZSkJXZwgq43iyywjVI9ZgA== |
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.
Had to update to 4.x because of FezVrasta/react-resize-aware#58
FYI @mfendeksilverstripe please take a look at the changes I've made when you get a chance and validate if they work as expected. |
Thanks @GuySartorelli will look into this early next week ;-) |
It's looking good @GuySartorelli, have covered all my tests cases: History view✅ I can see history tab being populated History detail✅ I can see history detail when click on individual history items Compare view✅ I can see the difference between published versions of my choosing Excellent work and big thanks for your help. I will merge this change-set as is but will keep the version 3 untagged. As a next step I will ask this to be tried out on a real project to pick up any potential issues. Happy to keep the failing JS tests of of scope of this change-set as this is tracked in #90 and can be actioned separately. |
@mfendeksilverstripe cc @GuySartorelli any chance of a tag for this merge? or are we still testing before that happens? |
@jareddreyerss This is not a supported module so I don't look after this one. @mfendeksilverstripe merged it so probably has access to tag. |
Hey @jareddreyerss tagged release is currently blocked by a critical issue #104 We plan to tag a release right after that one is resolved. |
NEW: CMS 5 upgrade.
ACs
Remaining items
Notes
Work done so far:
calc()
)Related pull requests
Related issues