-
Notifications
You must be signed in to change notification settings - Fork 41
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
Remove global issues page and start using popup instead #886
Conversation
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
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. |
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 outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
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
This comment was marked as resolved.
This comment was marked as resolved.
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.
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 :)
web/src/index.js
Outdated
@@ -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 />} /> |
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.
Actually, we do not need this route. Sorry, I overlooked it in previous reviews.
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) |
I've made two changes of note,
|
Of course, I don't mind at all.
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. 👍 |
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 know you had realize as well. |
Issues Dialog is not a page and therefore the route is not needed
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
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.
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.
Hey @dgdavid, Thanks a lot for the kind words.
Not at all, as you said Agama is a live project and changing things on the fly is quite alright.
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.
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.
Hey @dgdavid, If you find the time would you mind checking this out:
Thanks |
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,
Of course, ask whatever doubt you have. |
Great, thank you.
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
JFYI, this pull request was mentioned in the announcement of Agama 8. Thanks for your contribution! |
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
Screenshots