Popup Component and dlgEnter Implementation #5878
Conversation
updating my master
updating my master
updating my master
|
@Ivanluv Please could you review this? thanks |
|
@Ivanluv Did you get a chance to look at this? thanks |
|
@lloyddewit am looking at this now |
|
@lloyddewit the code looks fine from my review |
|
@Ivanluv Thank you for the peer review! @rdstern Above, @Patowhiz requested '@rdstern as discussed in issue #5596 the only thing waiting in the dialog are the samples and their tooltips. I will add them as soon as I get your feedback.`. If you are able to provide these then I can do the final review and merge, thanks |
There was a problem hiding this comment.
This is a good addition. It is incomplete as we know, but could have been merged. Except that the popup does work. However, the examples that are given do not move into the Data field - which incidentally should be better lined up and also have a colon. I suggest that is needed, before merging. It would do no harm to merge if needed for other pull requests, But otherwise it needs more work.
This is is now fixed, was a bug in checking for selection.
|
|
Just that the label "Data" should move up slightly and become "Data:" |
|
@rdstern that's now fixed. You can test the changes and @lloyddewit can do the review |
|
@rdstern Please could you review? thanks |
There was a problem hiding this comment.
@Patowhiz this would be ok to approve. But as you don't mind doing extra work now, I suggest you do 2 more steps on these changes before handing it over to @Wycklife . He is doing his exams just now.
In #5596 item a) was to swap the order of the new button and the the checkbox.
Then - more important - is to replace the examples you have, with ones I have provided, and add the tooltips.
You could stop there and we'll merge.
There was a problem hiding this comment.
@Patowhiz Thank you, I found the code structured and readable. Thank you also for the helpful commenting. I think this is particularly important for the new popup class because this will be used by others.
I think this class will be even easier for others to use if the non-XML comments (for public methods and data members) are converted to XML comments.
if you can do this then we can merge. Thanks
| ' | ||
| ' You should have received a copy of the GNU General Public License | ||
| ' along with this program. If not, see <http://www.gnu.org/licenses/>. | ||
| Public Class clsPopup |
There was a problem hiding this comment.
Please add XML comment for class (see RCodeStructure for example)
Co-authored-by: lloyddewit <[email protected]>
|
@rdstern Only the comments changed since your previous approval. Please could you approve again? thanks |
Fixes #5596 . Fixes #5876.
@africanmathsinitiative/developers this is ready for review.
This issue implements the popup component and also demonstrates through dlgEnter how other dialogs can use it. It's a continuation of PR #5877(which should be merged first) which has an empty class that represents the popup component.
Once merged, the other dialogs that currently have code that is implemented in this component, can be amended to use it.
@rdstern as discussed in issue #5596 the only thing waiting in the dialog are the samples and their tooltips. I will add them as soon as I get your feedback.