Skip to content
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

[#528] Feature/improve plan template popup #257

Merged
merged 2 commits into from
Jun 15, 2022

Conversation

vaszig
Copy link
Contributor

@vaszig vaszig commented Jun 3, 2022

No description provided.

@vaszig vaszig requested review from JostCrow and annashamray and removed request for JostCrow June 3, 2022 12:06
Copy link
Contributor

@annashamray annashamray left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since it's a styling issue I'll be a little nitpicking

  • The margin between title (plan name) and the body below is bigger in the design
  • The first column (preview__title) is too wide and it makes the data in the right column to look more crowded
  • The file url ("bestand") should be clickable
  • "X" button is missing in the top-right corner
  • (nice-to-have) I think the popup in the design is a little wider, and with a lot of action templates the wider popup could look nicer, but I don't insist on this one
  • (nice-to-have) I'd like to close modal by clicking outside of it. For now if you click outside you can even select radio button which is not nice. But it can be dealt with in another issue

I'll attach the screenshot with my modal so you can double check that it's displayed as expected

image

@vaszig
Copy link
Contributor Author

vaszig commented Jun 10, 2022

Concerning the comments 1,4 and 6:

  • The margin-padding is the one that we have for the modal generally, should I change it for this page specifically?

  • I didn't add the "x" button because I thought we should be consistent we the other popups as well (they don't have this button, only the "sluiten" one and you cannot click outside the popup). So, I believe that I should either change the other ones as well or keep it as it is.

@annashamray
Copy link
Contributor

annashamray commented Jun 15, 2022

  • Margin after the header is not something critical. We can leave it as it is
  • I personally think that 'x' button would be nice everywhere, but we can park this change for now

@vaszig
Copy link
Contributor Author

vaszig commented Jun 15, 2022

Yes, Jorik agrees with adding the "x" button as well, so I will change this.

Copy link
Member

@alextreme alextreme left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreeing with Anna here, the 'x' isn't necessary the work which is done in this PR is sufficient in my eyes too.

@alextreme alextreme merged commit 3b8cf00 into develop Jun 15, 2022
@alextreme alextreme deleted the task-528-improve-plan-template-popup branch June 15, 2022 17:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants