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

Feat/navbar overhaul #2072

Merged
merged 5 commits into from
Mar 22, 2022
Merged

Feat/navbar overhaul #2072

merged 5 commits into from
Mar 22, 2022

Conversation

acezard
Copy link
Contributor

@acezard acezard commented Mar 17, 2022

Linked to the react-native feature https://trello.com/c/KWxWhWBK/274-gestion-des-navbar-et-statusbar-dans-lensemble-de-laa-4-jh

This PR will handle react-native UI changes (statusbar, navbar) according to UI changes in the webapp rendered in the webview. This is done by calling cozy-intent to send the UI to display to react-native. There is last-state persistence so it is not strictly necessary to send a full object in the message, it is possible to only set the values that we want to be changed

Dialog

✅ Create shortcut
✅ Share
✅ Move to
✅ Categorize
✅ History
✅ Delete element confirmation

ActionMenu

✅ Sort files
✅ Cozy-bar "More" button
✅ FileAction button
✅ Upload file from OS

Viewer

✅ FileViewer
✅ FileViewer bottom drawer

Sidebar

✅ Mobile view

SelectionBar

✅ Delete element after selection

Issues:

  • TopBackground can't be determined with runtime palette (cozy-bar is different context)

Copy link
Contributor

@trollepierre trollepierre left a comment

Choose a reason for hiding this comment

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

💪

react/ActionMenu/index.jsx Outdated Show resolved Hide resolved
react/Dialog/index.jsx Show resolved Hide resolved
react/SelectionBar/index.jsx Show resolved Hide resolved
react/Dialog/index.jsx Outdated Show resolved Hide resolved
react/ActionMenu/index.jsx Outdated Show resolved Hide resolved
react/Viewer/PdfJsViewer.jsx Outdated Show resolved Hide resolved
- Update ESLint
- Update babel-preset
- Update babel-core
- Update configurations everywhere it's needed
- Fix ESLint issues after update
@acezard acezard force-pushed the feat/navbar-overhaul branch 6 times, most recently from 7ed8024 to 32f7b52 Compare March 21, 2022 16:18
Copy link
Collaborator

@JF-Cozy JF-Cozy left a comment

Choose a reason for hiding this comment

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

Pour les modifs suites aux remarques sur la PR, est-ce qu'on peut si possible squasher pour remplacer le code ? Faire une deuxième relecture presque 1 semaine après sans ça complique un peu la chose

react/ActionMenu/index.jsx Outdated Show resolved Hide resolved
react/ActionMenu/index.jsx Outdated Show resolved Hide resolved
react/SelectionBar/index.jsx Show resolved Hide resolved
react/Viewer/PdfJsViewer.jsx Outdated Show resolved Hide resolved
.eslintrc.json Show resolved Hide resolved
bottomOverlay: 'rgba(0, 0, 0, 0.5)',
topOverlay: 'rgba(0, 0, 0, 0.5)',
bottomOverlay: styles.root.backgroundColor,
topOverlay: styles.root.backgroundColor,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Pourquoi est-ce qu'on ne laisse pas les valeurs en dur ? Je trouve piégeux d'aller taper sur les valeurs de Mui, ça donne l'impression que c'est reactif au thème alors que ça ne l'est pas.

Si on n'a pas les valeurs dans des variables sémantiques, il faut essayer de les faire, mais sinon la valeur en dur est la bonne approche (ou la valeur issue de la palette)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

bien le string en question est déjà hardcoded dans la lib material ui, je pense qu'entre les deux autant prendre directement dans mui au moins on sait d'où vient la value

Copy link
Collaborator

Choose a reason for hiding this comment

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

je pense que non, Mui est notre base sur laquelle on s'appuie pour construire nos composants et nos thèmes, mais on override la plupart des choses questions style d'une part, et de plus tout ce qui est style est géré à "un" seul endroit : le makeTheme et la palette stylus. Je pense qu'on devrait éviter d'introduire une troisième source d'info. Par conséquent si on ne pioche pas dans notre unique source, on devrait plutôt écrire la valeur en dur.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

je revert

@@ -92,7 +95,7 @@
"cozy-device-helper": "^1.16.1",
"cozy-doctypes": "^1.69.0",
"cozy-harvest-lib": "^6.7.3",
"cozy-intent": "^1.13.0",
"cozy-intent": "1.13.0",
"cozy-sharing": "^3.10.0",
"cozy-stack-client": "27.19.1",
"css-loader": "0.28.11",
Copy link
Collaborator

Choose a reason for hiding this comment

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

idem que précédemment : pourquoi ce commit est nécessaire ici ? Est-ce qu'on peut ajouter de la description dans le commit message ?

@@ -92,7 +95,7 @@
"cozy-device-helper": "^1.16.1",
"cozy-doctypes": "^1.69.0",
"cozy-harvest-lib": "^6.7.3",
"cozy-intent": "^1.13.0",
"cozy-intent": "1.13.0",
Copy link
Collaborator

Choose a reason for hiding this comment

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

est-ce que le remplacement de ^1.13.0 par 1.13.0 est voulu ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes il y a un probleme de resolution de path sur la 1.13.3

Copy link
Collaborator

Choose a reason for hiding this comment

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

ah. Peut-on linker l'issue peut-être ? Est-il prévu après la résolution de l'issue de faire une 1.13.4 débuggué et repasser cozy-intent en ^1.13.4 dans cozy-ui ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes j'ai déjà commencé la résolution pour cela je pense qu'il est redondant de faire une issue

package.json Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
bottomOverlay: 'rgba(0, 0, 0, 0.5)',
topOverlay: 'rgba(0, 0, 0, 0.5)',
bottomOverlay: styles.root.backgroundColor,
topOverlay: styles.root.backgroundColor,
Copy link
Collaborator

Choose a reason for hiding this comment

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

je pense que non, Mui est notre base sur laquelle on s'appuie pour construire nos composants et nos thèmes, mais on override la plupart des choses questions style d'une part, et de plus tout ce qui est style est géré à "un" seul endroit : le makeTheme et la palette stylus. Je pense qu'on devrait éviter d'introduire une troisième source d'info. Par conséquent si on ne pioche pas dans notre unique source, on devrait plutôt écrire la valeur en dur.

@@ -92,7 +95,7 @@
"cozy-device-helper": "^1.16.1",
"cozy-doctypes": "^1.69.0",
"cozy-harvest-lib": "^6.7.3",
"cozy-intent": "^1.13.0",
"cozy-intent": "1.13.0",
Copy link
Collaborator

Choose a reason for hiding this comment

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

ah. Peut-on linker l'issue peut-être ? Est-il prévu après la résolution de l'issue de faire une 1.13.4 débuggué et repasser cozy-intent en ^1.13.4 dans cozy-ui ?

@@ -84,7 +84,7 @@ html
--slateGrey #5D6165
--charcoalGrey #32363F
--black palette.Common['black']
--overlay rgba(50, 54, 63, .5)
--overlay rgba(50, 54, 63, .5) // TODO: refactor to semantic variable
Copy link
Collaborator

Choose a reason for hiding this comment

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

Le commentaire est ambigu ici ce n'est pas déconnant. Bref supprimons le todo et avançons :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  • [ ]

.eslintrc.json Show resolved Hide resolved
Also add minor changes to improve codebase
@acezard acezard merged commit 5df4e2b into master Mar 22, 2022
@acezard acezard deleted the feat/navbar-overhaul branch March 22, 2022 15:06
@cozy-bot
Copy link

🎉 This PR is included in version 62.2.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants