Remove global issues page and start using popup instead #886

balsa-asanovic · 2023-11-23T00:20:58Z

Problem

"We have agreed to use the sidebar for actions related to the installer itself (e.g., diagnosis tools, display lang, etc). Everything related to the target system to install should not be placed in the siderbar.

The only thing related to the target system still present in the sidebar is the issues link. The issues should be globally accessible from another place (footer, header, ...)."

As described in #872

This PR fixes #872

Solution

Issues page link is removed from the sidebar and all other places where it was present.

During the progress of this PR, decision was made to drop the showing of global (all) issues at once in the popup.

Because of this, the Issues popup (previously a page) can no longer be accessed from the sidebar (or from the icon in the header, which was an idea at some point).

The Issues Dialog (popup) now shows issues related only to a single section (software, product, storage, ...) and is opened by clicking on the warning link in the sections on the overview page (and a few other places).

For this reason, the prop id for the Section component was introduced, so the issues could be filtered to a specific type for the popup based on from where has the user clicked the warning.

Testing

Tested manually
Adjusted unit test files to some extent, possibly more work is needed here.

Screenshots

Also removed notification mark

Removed title prop as popover is no longer used. Added sectionName prop used to determine highlighting on the issues popup

Added prop for closing popup Added sectionHighlight prop used to determine which section to highlight Added autoscroll to the appropriate section based on sectionHighlight

Added sectionName prop to determine which section is rendering the comp Removed title prop as it is not used anymore

dgdavid · 2023-11-23T19:24:20Z

@balsa-asanovic,

Just a comment to say thank you. I'll do a first review ASAP. Not sure if tomorrow I'll have time, but for sure it's in the top of my todo list for the beginning of next week.

web/src/components/core/Sidebar.jsx

into extract-issues

added class for warning icon color

This ref is passed to section comp which now takes it as a prop

Uses orange color for warning icon

In the context of improving the core/Page component and how the Agama layout is handled, it's needed to have a way of opening the Sidebar from outside instead of keeping it responsible of rendering a button for such an action. Using a prop to do so following the recomendation found in the "Pitfall" warning of "Exposing your own imperaive methods" section in the React#useImpreativeHandle documentation. See [1] Please, note that this commit also drops the logic for displaying a "notification mark" because there is already a work in progress for getting ride of such a feature once we have decided that the sidebar is for placing Agama stuff only. See [2] [1] https://react.dev/reference/react/useImperativeHandle#exposing-your-own-imperative-methods [2] #886

dgdavid

@balsa-asanovic

Thanks a lot for addressing the latest set of comments/suggestions. Much appreciated.

I hope you don't mind I've sent some commits this morning. Otherwise, please excuse me for doing so. Reason was not other than speeding up this PR in order to allow you jump to another one if you want 😇 I thought it was quickly than writing another bunch of comments. Furthermore, the latest ones are things that I realized when working in the formers.

I'll explain some things in a while, in a comment below.

BTW, time of adding the entry in the changelog :)

dgdavid · 2024-01-04T09:03:13Z

web/src/index.js

@@ -84,7 +84,7 @@ root.render(
 <Route path="/storage/zfcp" element={<ZFCPPage />} />
 <Route path="/network" element={<NetworkPage />} />
 <Route path="/users" element={<UsersPage />} />
- <Route path="/issues" element={<IssuesPage />} />
+ <Route path="/issues" element={<IssuesDialog />} />


Actually, we do not need this route. Sorry, I overlooked it in previous reviews.

dgdavid · 2024-01-04T11:50:11Z

BTW, time of adding the entry in the changelog :)

And also, if you don't mind, time for updating the PR description since it is a bit outdated.

Not sure if you noticed it, but we configured the repo for including it as the commit message by default (See https://github.blog/changelog/2022-08-23-new-options-for-controlling-the-default-commit-message-when-merging-a-pull-request/ to know more)

dgdavid · 2024-01-04T13:24:38Z

I'll explain some things in a while, in a comment below.

I've made two changes of note,

Drop the core/Section component from the IssuesDialog

It was me who proposed using it, my fault. However, when taking a look at the final result, I realized that visually it does not look quite well when rendered inside a Popup without using a section title and icon because at this time Section reserves the space for the icon. Not sure if in the future the component will be more flexible and allows conditionally to reserve the spacer or not, but for now not using it here is the straight forward solution.

Before After

Note that, additionally, now issues are wrapped by a plain HTML ul element instead of a collection of nested divs. IMHO, semantically more accurate: it is a list of issues, at the end.
Make the ValidationErrors component able to infer the issues area from the given sectionId.

This is needed because the IssuesDialog can be now open from sections sub-headers and it will display the errors given area based on the id of the section. Maybe is something to improve, at least semantically. But in another PR. For now, the change introduced at e1b7f79 allows displaying the issues for the same area from different sections without forcing all of them to have the same id, which will be wrong if sections are rendered in the same page. For example, from the StorePage the Actions section allows now opening such a dialog by using a meaningful storage-actions id. A bit tricky, indeed, but enough for considering this PR ready.

Before (without id) After (using storage-actions

balsa-asanovic · 2024-01-04T14:15:46Z

I hope you don't mind I've sent some commits this morning. Otherwise, please excuse me for doing so.

Of course, I don't mind at all.

I realized that visually it does not look quite well when rendered inside a Popup

This was exactly something I noticed last night when making the changes.

I thought about suggesting using some sort of bullets, but at the end didn't leave a comment as to not extend the PR more.

Glad to see you went in the similar direction. 👍

dgdavid · 2024-01-04T15:13:52Z

This was exactly something I noticed last night when making the changes.

I thought about suggesting using some sort of bullets, but at the end didn't leave a comment as to not extend the PR more.

Please, in the future, do not hesitate on making as many comments or suggestions as you want. I'm pretty sure they will help the project to go in the right direction. Don't worry about the "pr extension", we can help to move it forward at any time.

Glad to see you went in the similar direction. 👍

Glad to know you had realize as well.

Issues Dialog is not a page and therefore the route is not needed

balsa-asanovic · 2024-01-04T21:54:04Z

Okay, so I removed the Issues Dialog route from index file.

I've updated the PR title and description, feel free to adjust this further if you notice something wrong or missing.

Also, I've added the entry to the changes file.

Using margin instead of padding

dgdavid

@balsa-asanovic,

Thanks for your hard work on this 💯

Definitely, it has been a PR quite longer than expected, mostly because we changed our minds about the final result while reviewing your remarkable contribution. I understand how frustrating that can be, but you've been incredibly patient and broad-minded understanding we are working on live project where some things are re-defined on the fly. Your willingness to change almost everything again surprised me. I hope at least you've had good coding moments and learned something along the way.

As you have written, there is still a bit of work to do here and there. Nevertheless, the PR looks good to me 👍 as it is, especially taking into account that these tweaks will come once we have them more clear by gathering feedback from both developers and users.

Needless to say, looking forward to reviewing your next PR for the Agama project. I'm curious about which issues you're gonna pick 😜

See ya.

balsa-asanovic · 2024-01-05T12:28:47Z

Hey @dgdavid,

Thanks a lot for the kind words.

I understand how frustrating that can be

Not at all, as you said Agama is a live project and changing things on the fly is quite alright.

I hope at least you've had good coding moments and learned something along the way.

I enjoyed working on this and picked up quite a few new things (I always do here at Agama).

In large part this is due to your reviews and very detailed comments, I realize that sometimes just doing something yourself would be easier and faster, so thank you a lot for you willingness to always help.

Needless to say, looking forward to reviewing your next PR for the Agama project. I'm curious about which issues you're gonna pick 😜

Haha, if you have some good idea about an issue (or a planned feature) on which I could work, please do make a suggestions.

If not, here's something of interest I noticed after a quick read through the issues:

#870 This seems interesting, however I'd need a bit more of an explanation than currently available in the issue description.

#112 This is an older issue. I mentioned already at one point that I could take it, it seems that it is still available.

Some other recent issues seem to be already taken by new contributors.

But, again, if you have a better idea than this, please do say, these are just suggestions.

## Problem The `ul` styles were overwritten in #886 to make them looks like what they are: unordered lists. However, such a changed impacted in the look&feel of the current Agama selectors. ## Solution Restore the Agama selectors by setting their list-type and margins explicetely. ## Testing - Tested manually

Because it is kind of dead code since since #886, where it was overlooked and not removed.

Because it is kind of dead code since #886, where it was overlooked and not removed.

Drops the core/`NotificationMark` component and its styles since they are no longer used since #886.

balsa-asanovic · 2024-01-11T21:00:37Z

Hey @dgdavid,

If you find the time would you mind checking this out:

Haha, if you have some good idea about an issue (or a planned feature) on which I could work, please do make a suggestions.

If not, here's something of interest I noticed after a quick read through the issues:

#870 This seems interesting, however I'd need a bit more of an explanation than currently available in the issue description.

#112 This is an older issue. I mentioned already at one point that I could take it, it seems that it is still available.

Some other recent issues seem to be already taken by new contributors.

But, again, if you have a better idea than this, please do say, these are just suggestions.

Thanks

dgdavid · 2024-01-12T10:57:44Z

Sorry @balsa-asanovic, I had forgotten to answer to your question. Thanks for the friendly ping :)

After discussing it with @imobachgs, we believe the #112 is the good candidate since,

It's old enough 🙈
It's completely about the front-end
You already have provided a lot of feedback there 💯
As you pointed out, Add provider for issues #870 was not well-defined. In fact, I've moved it to a discussion

Of course, ask whatever doubt you have.

balsa-asanovic · 2024-01-12T11:14:59Z

Great, thank you.

Of course, ask whatever doubt you have.

Will do 😄

Prepare for releasing Agama 8. It includes the following pull requests: * #884 * #886 * #914 * #918 * #956 * #957 * #958 * #959 * #960 * #961 * #962 * #963 * #964 * #965 * #966 * #969 * #970 * #976 * #977 * #978 * #979 * #980 * #981 * #983 * #984 * #985 * #986 * #988 * #991 * #992 * #995 * #996 * #997 * #999 * #1003 * #1004 * #1006 * #1007 * #1008 * #1009 * #1010 * #1011 * #1012 * #1014 * #1015 * #1016 * #1017 * #1020 * #1022 * #1023 * #1024 * #1025 * #1027 * #1028 * #1029 * #1030 * #1031 * #1032 * #1033 * #1034 * #1035 * #1036 * #1038 * #1039 * #1041 * #1042 * #1043 * #1045 * #1046 * #1047 * #1048 * #1052 * #1054 * #1056 * #1057 * #1060 * #1061 * #1062 * #1063 * #1064 * #1066 * #1067 * #1068 * #1069 * #1071 * #1072 * #1073 * #1074 * #1075 * #1079 * #1080 * #1081 * #1082 * #1085 * #1086 * #1087 * #1088 * #1089 * #1090 * #1091 * #1092 * #1093 * #1094 * #1095 * #1096 * #1097 * #1098 * #1099 * #1100 * #1102 * #1103 * #1104 * #1105 * #1106 * #1109 * #1110 * #1111 * #1112 * #1114 * #1116 * #1117 * #1118 * #1119 * #1120 * #1121 * #1122 * #1123 * #1125 * #1126 * #1127 * #1128 * #1129 * #1130 * #1131 * #1132 * #1133 * #1134 * #1135 * #1136 * #1138 * #1139 * #1140 * #1141 * #1142 * #1143 * #1144 * #1145 * #1146 * #1147 * #1148 * #1149 * #1151 * #1152 * #1153 * #1154 * #1155 * #1156 * #1157 * #1158 * #1160 * #1161 * #1162 * #1163 * #1164 * #1165 * #1166 * #1167 * #1168 * #1169 * #1170 * #1171 * #1172 * #1173 * #1174 * #1175 * #1177 * #1178 * #1180 * #1181 * #1182 * #1183 * #1184 * #1185 * #1187 * #1188 * #1189 * #1190 * #1191 * #1192 * #1193 * #1194 * #1195 * #1196 * #1198 * #1199 * #1200 * #1201 * #1203 * #1204 * #1205 * #1206 * #1207 * #1208 * #1209 * #1210 * #1211 * #1212 * #1213 * #1214 * #1215 * #1216 * #1217 * #1219 * #1220 * #1221 * #1222 * #1223 * #1224 * #1225 * #1226 * #1227 * #1229

ancorgs · 2024-05-17T18:15:46Z

JFYI, this pull request was mentioned in the announcement of Agama 8.

Thanks for your contribution!

balsa-asanovic added 8 commits November 18, 2023 22:23

removed IssuesLink from App.jsx

c520b56

added class for highlighting

47d04f4

added warning button to siderbar

65b2ccb

Also removed notification mark

added className prop for styling

8a68dcc

Removed title prop as popover is no longer used. Added sectionName prop used to determine highlighting on the issues popup

removed title prop from ValidationErrors

3754f47

converted page to popup

c73c8cd

Added prop for closing popup Added sectionHighlight prop used to determine which section to highlight Added autoscroll to the appropriate section based on sectionHighlight

Removed popover, now it opens Issues popup

951fe01

Added sectionName prop to determine which section is rendering the comp Removed title prop as it is not used anymore

typo correction in prop naming

e5e85bc

dgdavid self-requested a review November 23, 2023 19:22

dgdavid self-assigned this Nov 23, 2023

balsa-asanovic commented Nov 27, 2023

View reviewed changes

web/src/components/core/Sidebar.jsx Outdated Show resolved Hide resolved

This comment was marked as outdated.

Sign in to view

balsa-asanovic added 10 commits December 7, 2023 22:29

changed name of comp IssuesPage to IssuesDialog

dfdfd9d

changed comp name IssuesPage to IssuesDialog

eff9b15

Merge branch 'extract-issues' of https://github.com/balsa-asanovic/agama

3195346

into extract-issues

Reverting changes to PO files due to conflicts

3df181b

gemfile.lock reverts

4b92d6b

changed color for highlighting

3f011a0

added class for warning icon color

added new variable for warning icon color

8223e4f

switched to using a single ref hook

d916474

This ref is passed to section comp which now takes it as a prop

Section component now uses forwardRef

cd0a59e

changed the look of the issues button

ef57b49

Uses orange color for warning icon

This comment was marked as resolved.

Sign in to view

dgdavid reviewed Jan 4, 2024

View reviewed changes

Merge branch 'openSUSE:master' into extract-issues

d328700

balsa-asanovic changed the title ~~Extract issues link from sidebar~~ Remove global issues page and start using popup instead Jan 4, 2024

balsa-asanovic added 2 commits January 4, 2024 22:39

removed route for Issues Dialog

0e84d9b

Issues Dialog is not a page and therefore the route is not needed

changes file update

75d87a1

dgdavid added 2 commits January 4, 2024 23:21

[web] Fix styles

a1a04a5

Using margin instead of padding

[web] Please ESLint

e443124

dgdavid approved these changes Jan 5, 2024

View reviewed changes

imobachgs approved these changes Jan 8, 2024

View reviewed changes

dgdavid merged commit b8b9e64 into agama-project:master Jan 8, 2024
1 check passed

balsa-asanovic deleted the extract-issues branch January 8, 2024 12:03

dgdavid mentioned this pull request Jan 8, 2024

[web] Fix Agama selectors look&feel #979

Merged

dgdavid added a commit that referenced this pull request Jan 8, 2024

[web] Drop NotificationMark component

984375a

Because it is kind of dead code since since #886, where it was overlooked and not removed.

dgdavid added a commit that referenced this pull request Jan 8, 2024

[web] Drop NotificationMark component

748b145

Because it is kind of dead code since #886, where it was overlooked and not removed.

dgdavid mentioned this pull request Jan 8, 2024

[web] Drop core/NotificationMark component #980

Merged

dgdavid added a commit that referenced this pull request Jan 8, 2024

[web] Drop core/NotificationMark component (#980)

cf99ad4

Drops the core/`NotificationMark` component and its styles since they are no longer used since #886.

imobachgs mentioned this pull request Feb 12, 2024

Update to release 8 #1037

Closed

imobachgs mentioned this pull request May 17, 2024

Update to release 8 #1232

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove global issues page and start using popup instead #886

Remove global issues page and start using popup instead #886

balsa-asanovic commented Nov 23, 2023 •

edited

Loading

dgdavid commented Nov 23, 2023

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as resolved.

dgdavid left a comment •

edited

Loading

dgdavid Jan 4, 2024

dgdavid commented Jan 4, 2024 •

edited

Loading

dgdavid commented Jan 4, 2024

balsa-asanovic commented Jan 4, 2024

dgdavid commented Jan 4, 2024 •

edited

Loading

balsa-asanovic commented Jan 4, 2024

dgdavid left a comment •

edited

Loading

balsa-asanovic commented Jan 5, 2024

balsa-asanovic commented Jan 11, 2024

dgdavid commented Jan 12, 2024

balsa-asanovic commented Jan 12, 2024

ancorgs commented May 17, 2024

Remove global issues page and start using popup instead #886

Remove global issues page and start using popup instead #886

Conversation

balsa-asanovic commented Nov 23, 2023 • edited Loading

Problem

Solution

Testing

Screenshots

dgdavid commented Nov 23, 2023

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as resolved.

dgdavid left a comment • edited Loading

Choose a reason for hiding this comment

dgdavid Jan 4, 2024

Choose a reason for hiding this comment

dgdavid commented Jan 4, 2024 • edited Loading

dgdavid commented Jan 4, 2024

balsa-asanovic commented Jan 4, 2024

dgdavid commented Jan 4, 2024 • edited Loading

balsa-asanovic commented Jan 4, 2024

dgdavid left a comment • edited Loading

Choose a reason for hiding this comment

balsa-asanovic commented Jan 5, 2024

balsa-asanovic commented Jan 11, 2024

dgdavid commented Jan 12, 2024

balsa-asanovic commented Jan 12, 2024

ancorgs commented May 17, 2024

balsa-asanovic commented Nov 23, 2023 •

edited

Loading

dgdavid left a comment •

edited

Loading

dgdavid commented Jan 4, 2024 •

edited

Loading

dgdavid commented Jan 4, 2024 •

edited

Loading

dgdavid left a comment •

edited

Loading