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

fix: Clear console errors #244

Merged
merged 1 commit into from
Dec 22, 2021
Merged

fix: Clear console errors #244

merged 1 commit into from
Dec 22, 2021

Conversation

acezard
Copy link
Contributor

@acezard acezard commented Dec 16, 2021

Clearing small bugs that polluted the console. There is still one:

image

But it's not possible to fix it from cozy-notes, it's a cozy-sharing issue.

package.json Outdated
@@ -57,7 +57,7 @@
"dependencies": {
"@atlaskit/icon": "21.1.0",
"@atlaskit/media-core": "32.2.0",
"@material-ui/core": "4.12.3",
"@material-ui/core": "4.11.4",
Copy link
Contributor

Choose a reason for hiding this comment

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

why this downgrade?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it is required with the version of cozy-ui we use

const EditorCorner = ({ doc, isPublic, isReadOnly, title }) => {
const { t } = useI18n()
if (!isPublic) return <SharingWidget file={doc.file} title={title} />
else if (isReadOnly) {
return (
<Tooltip title={t('Notes.Editor.read_only')}>
<Icon icon="lock" color="var(--primaryTextColor)" />
<ForwardedIcon icon="lock" color="var(--primaryTextColor)" />
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not updating cozy-ui in order to forwardRef by default on Icon? 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't remember why I didn't choose to update cozy-ui, maybe there were more problems. I can do that though

Copy link
Contributor

Choose a reason for hiding this comment

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

Did you created the issue on cozy-ui? Can you link it in the code?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I didn't create an issue yet, the build is failing with the latest cozy-ui for a different reason (konnector related)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I mean, I'm not sure the forwardRef error exists with the latest cozy-ui (that doesn't work on cozy-notes right now, I prefered delivering the fixes instead)

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm pretty sur the issue still exist in the latest UI's version

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will check on the latest version after cozy-notes is delivered

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Crash--
Copy link
Contributor

Crash-- commented Dec 20, 2021

Did you created the issue of cozy-sharing side? It is already created so you can link to it form here?

@acezard
Copy link
Contributor Author

acezard commented Dec 20, 2021

màj cozy-ui
issue sur cozy-ui si forwardref

@acezard
Copy link
Contributor Author

acezard commented Dec 20, 2021

Update: cozy-ui update is too complex, leaving it as is for now

@acezard acezard merged commit dc41656 into master Dec 22, 2021
@delete-merged-branch delete-merged-branch bot deleted the fix/clear-console-errors branch December 22, 2021 08:05
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