`Development`: Enhance Share Modal of Apollon standalone #34

minrows · 2022-05-17T10:23:11Z

Motivation and Context

This PR enhances the currently existing share modal of Apollon Standalone.
The current version of the Share modal requires 4 clicks to be done before sharing a link, which is a time-consuming task.
This proposed share modal reduces the number of clicks to 2.

Steps for Testing

To test locally:

Clone the repo
checkout the enhancement/change-share-modal branch
yarn install && yarn start
Once the application is running, go to the share menu and observe the newly implemented buttons instead of the old dropdown menu entries.

To test in test server:

Goto https://test1.apollon.ase.in.tum.de

Screenshot:

Before:

After:

Todo:

Deploy in test server

…into enhancement/change-share-modal

Dominik-Weinzierl

Looks good to me. Some minor suggestions:

When I shrink the window to the minimum width, the text exceeds the button:
I think it would be better if we center the entire row (or shrink the pop up):

Dominik-Weinzierl · 2022-05-17T18:33:10Z

Works now 👍 Well done

…oring for darkmode tooltip

packages/webapp/src/main/components/modals/share-modal/share-modal.tsx

…om declaration type

ole-ve

Tested on Apollon-TS1, works as expected. Nice improvement

canberkanar

Tested on Apollon test server on the testing session. The new share modal is much more convenient in my opinion. Nicely done!

akesfeden

Tested on test server 1 during testing session, new modal seems good and simple. We talked about improving the error message saying "Are you sure it contains a diagram.json?" when and empty model is shared but it is decided that this is outside of the scope of this PR.

Lugitan

Tested on Apollon-TS1, works as described. Very nice improvement.

Dominik-Weinzierl

It seems like the icon is not centered - i am not sure if this is intended or not 🤔

And you define colors in multiple files e.g. in packages/webapp/src/main/styles.css and in packages/webapp/src/main/themings.json - i guess it would be better to define those colors only at one place (as part of another PR).

You can also think about to extract the text into an additional file - e.g. to support multiple languages in the future.

But thanks for incorporating my suggestions 👍

bassner

Seems like this has already been tested by enough people, so I had a look at the code and got a few remarks:

packages/webapp/src/main/components/modals/share-modal/share-modal.tsx

packages/webapp/src/main/styles.css

bassner · 2022-05-23T13:37:59Z

packages/webapp/src/main/styles.css

@@ -70,16 +91,16 @@ body {
  cursor: pointer;
 }

-.theme-toggle.dark .sun-and-moon > .sun-beams {
+.theme-toggle.dark [class$='sun-and-moon'] > [class$='sun-beams'] {


That looks weird. Why can't we keep the normal class selector?

"svgr webpack" package added prefix "tooltip_svg__" to the classname that I assigned.
Since I did not have control over it and was not sure that's the constant prefix that will be added, I thought of using the selector that selects the class ending with the class name that I assigned.

…odal.tsx Co-authored-by: Patrick Bassner <[email protected]>

Co-authored-by: Patrick Bassner <[email protected]>

minrows · 2022-05-23T23:54:14Z

It seems like the icon is not centered - i am not sure if this is intended or not 🤔

And you define colors in multiple files e.g. in packages/webapp/src/main/styles.css and in packages/webapp/src/main/themings.json - i guess it would be better to define those colors only at one place (as part of another PR).

You can also think about to extract the text into an additional file - e.g. to support multiple languages in the future.

But thanks for incorporating my suggestions 👍

Hi @Dominik-Weinzierl,

The icon, currently, is aligned centrally with the description of texts. And IMO, is okay as it is now. Do you want me to align as shown in the snippet (b)?
(a)
(b)
Regarding the color definition, themings.json is responsible for defining and injecting colors for dark/light mode while style.css is responsible for setting them. If the colors are common for both dark/light modes, that's when we define the color in the style.css file. But yes, this can be discussed and changed in other PR if needed.
Thanks for your suggestion, could be a next possible feature to be implemented in Standalone. 👍

martindevtum · 2022-05-24T18:15:04Z

packages/webapp/webpack/webpack.common.js

@@ -47,6 +47,10 @@ module.exports = {
        test: /\.css$/,
        use: ['style-loader', 'css-loader'],
      },
+      {
+        test: /\.svg$/,


Is that really necessary?

why don't we just include the svg as the src of a normal img tag. I think svgr/webpack is just an unnessecary additional dependency

or including the svgs via the < use >

We would then not be able to style it via an external CSS file, which is needed for theme-switcher icon.

martindevtum · 2022-05-24T18:15:36Z

packages/webapp/package.json

@@ -35,6 +35,7 @@
    "websocket": "1.0.34"
  },
  "devDependencies": {
+    "@svgr/webpack": "^6.2.1",


see my comment in the webpack config

martindevtum

Code looks good overall. My remarks regarding svgr are optional in my opinion.

minrows added 7 commits May 16, 2022 21:07

[MODIFIED] Initiated change of Share Modal

07cc735

[MODIFIED] Code refactoring and addition of Buttons/copy link layout

74f7690

[MODIFIED] Minor code refactoring

babf310

[MODIFIED/ADDED] Added toaster to notify user about the sharing link

103f52c

[MODIFIED] Updated toasts content and the way they are being displayed

b8e3826

[MODIFIED] Executed prettier

e425ca7

[MODIFIED] Minor code refactoring

250ba8c

minrows requested review from bassner, martindevtum and krusche May 17, 2022 10:23

minrows self-assigned this May 17, 2022

minrows added 5 commits May 17, 2022 16:33

Merge branch 'main' of https://github.com/ls1intum/Apollon_standalone …

eeb108e

…into enhancement/change-share-modal

[MODIFIED] Removal of box outside button container

4f79e08

[MODIFIED] Fix darkmode bug on Menu Entry Load Diagram

584efb7

[MODIFIED] Darkmode integration for Share Modal and Toastify

b4e3efe

[MODIFIED] Executed prettier

09efed6

Dominik-Weinzierl suggested changes May 17, 2022

View reviewed changes

minrows added 2 commits May 17, 2022 19:19

[MODIFIED] Share Modal is now made responsive

bc79d23

[MODIFIED] Change of Share Modal Text

40426f7

minrows added 4 commits May 17, 2022 21:12

[MODIFIED] Minor change in styling of share buttons

ac04866

[MODIFIED] Added tooltip in body of Share modal and minor code refact…

3d31c6d

…oring for darkmode tooltip

[MODIFIED] Executed Prettier

2118c00

[MODIFIED/ADDED] Icons are now used from svg files

e840224

Dominik-Weinzierl reviewed May 18, 2022

View reviewed changes

packages/webapp/src/main/components/modals/share-modal/share-modal.tsx Outdated Show resolved Hide resolved

minrows added 4 commits May 18, 2022 17:17

[MODIFIED] Removal of @ts-ignore while importing svg by defining cust…

53848cf

…om declaration type

[MODIFIED] Continuity to previous commit (removal of ts-ignore)

5987f98

[MODIFIED] Removal of relative path and Executed prettier

dbb3312

[MODIFIED] Continuity of previous commit (Removal of relative path)

b9a2316

ole-ve approved these changes May 23, 2022

View reviewed changes

canberkanar approved these changes May 23, 2022

View reviewed changes

akesfeden approved these changes May 23, 2022

View reviewed changes

Lugitan approved these changes May 23, 2022

View reviewed changes

Dominik-Weinzierl reviewed May 23, 2022

View reviewed changes

bassner requested changes May 23, 2022

View reviewed changes

minrows and others added 2 commits May 23, 2022 18:38

Update packages/webapp/src/main/components/modals/share-modal/share-m…

1b44fdd

…odal.tsx Co-authored-by: Patrick Bassner <[email protected]>

Update packages/webapp/src/main/styles.css

574e4a8

Co-authored-by: Patrick Bassner <[email protected]>

minrows requested a review from bassner May 23, 2022 16:46

minrows added 2 commits May 23, 2022 18:58

[MODIFIED] Aligned share tooltip icon to the center

edfd85d

[MODIFIED] Reverted previous commit

be0511c

martindevtum reviewed May 24, 2022

View reviewed changes

martindevtum approved these changes May 24, 2022

View reviewed changes

krusche merged commit afafa18 into main May 25, 2022

krusche deleted the enhancement/change-share-modal branch May 25, 2022 15:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

`Development`: Enhance Share Modal of Apollon standalone #34

`Development`: Enhance Share Modal of Apollon standalone #34

minrows commented May 17, 2022 •

edited

Loading

Dominik-Weinzierl left a comment •

edited

Loading

Dominik-Weinzierl commented May 17, 2022

ole-ve left a comment

canberkanar left a comment

akesfeden left a comment

Lugitan left a comment

Dominik-Weinzierl left a comment •

edited

Loading

bassner left a comment

bassner May 23, 2022

minrows May 23, 2022

minrows commented May 23, 2022

martindevtum May 24, 2022 •

edited

Loading

martindevtum May 24, 2022

minrows May 25, 2022 •

edited

Loading

martindevtum May 24, 2022

martindevtum left a comment

Development: Enhance Share Modal of Apollon standalone #34

Development: Enhance Share Modal of Apollon standalone #34

Conversation

minrows commented May 17, 2022 • edited Loading

Motivation and Context

Steps for Testing

Screenshot:

Todo:

Dominik-Weinzierl left a comment • edited Loading

Choose a reason for hiding this comment

Dominik-Weinzierl commented May 17, 2022

ole-ve left a comment

Choose a reason for hiding this comment

canberkanar left a comment

Choose a reason for hiding this comment

akesfeden left a comment

Choose a reason for hiding this comment

Lugitan left a comment

Choose a reason for hiding this comment

Dominik-Weinzierl left a comment • edited Loading

Choose a reason for hiding this comment

bassner left a comment

Choose a reason for hiding this comment

bassner May 23, 2022

Choose a reason for hiding this comment

minrows May 23, 2022

Choose a reason for hiding this comment

minrows commented May 23, 2022

martindevtum May 24, 2022 • edited Loading

Choose a reason for hiding this comment

martindevtum May 24, 2022

Choose a reason for hiding this comment

minrows May 25, 2022 • edited Loading

Choose a reason for hiding this comment

martindevtum May 24, 2022

Choose a reason for hiding this comment

martindevtum left a comment

Choose a reason for hiding this comment

`Development`: Enhance Share Modal of Apollon standalone #34

`Development`: Enhance Share Modal of Apollon standalone #34

minrows commented May 17, 2022 •

edited

Loading

Dominik-Weinzierl left a comment •

edited

Loading

Dominik-Weinzierl left a comment •

edited

Loading

martindevtum May 24, 2022 •

edited

Loading

minrows May 25, 2022 •

edited

Loading