D12571: Modernize Settings window

[Elvis Angelaccio]

elvisangelaccio requested changes to this revision.
elvisangelaccio added a comment.
This revision now requires changes to proceed.


  I'm ok with the UI changes now. 
  Just some small nitpicks left and then I think we can ship this.

INLINE COMMENTS

> behaviorsettingspage.cpp:27-31
> +#include <QButtonGroup>
>  #include <QCheckBox>
> -#include <QGroupBox>
>  #include <QRadioButton>
> -#include <QVBoxLayout>
> +#include <QFormLayout>
> +#include <QSpacerItem>

Please keep the sorting of includes

> behaviorsettingspage.cpp:77
> +
> +    topLayout->addItem(new QSpacerItem(0, 18, QSizePolicy::Fixed, QSizePolicy::Fixed));
>  

Please add a global variable instead of hardcoding 18 in different places.

/OT
Does this 18 come from some HIG? If yes, it would be a good idea to put it in some global config that can be shared across all KDE apps.
So that if one day we realize 18 is not good anymore and we want to use 19, we will have just one thing to update.

> startupsettingspage.cpp:52
> +    // create 'Home URL' editor
> +    QHBoxLayout* homeUrlBoxLayout = new QHBoxLayout();
>      homeUrlBoxLayout->setMargin(0);

Please add a parent to this layout

> startupsettingspage.cpp:69
>  
> -    QWidget* buttonBox = new QWidget(homeBox);
> -    QHBoxLayout *buttonBoxLayout = new QHBoxLayout(buttonBox);
> +    QHBoxLayout* buttonBoxLayout = new QHBoxLayout();
>      buttonBoxLayout->setMargin(0);

Please add a parent to this layout

REPOSITORY
  R318 Dolphin

REVISION DETAIL
  https://phabricator.kde.org/D12571

To: ngraham, #dolphin, #vdg, broulik, elvisangelaccio
Cc: cfeck, medhefgo, zzag, rkflx, kfm-devel, elvisangelaccio, abetts, spoorun, navarromorales, isidorov, firef, andrebarros, emmanuelp
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.kde.org/mailman/private/kfm-devel/attachments/20180702/845aa1a0/attachment.htm>