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

Project picker tile sizes #2236

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

z1ndabad
Copy link
Collaborator

@z1ndabad z1ndabad commented Apr 16, 2020

Closes #2102

Attempted both of the fixes suggested in the issue, but in the end I just set project-preview height relative to the viewport. Rationale was:

  • Adding a separate div to act as a spacer may muddy separation of style and content
  • My approaches for cleanly setting default names and timestamps broke function purity
  • Absence of a project name is a small (if messy) tell that something basic needs fixing

Ran this on a few different screen sizes to make sure the modal is still readable, but I might be wrong there.

@z1ndabad z1ndabad requested a review from outoftime April 19, 2020 06:30
Copy link
Contributor

@outoftime outoftime left a comment

Choose a reason for hiding this comment

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

Thanks @Tsamee! I'm not sure that setting the height of the element relative to the viewport is the right approach here—the two don't seem fundamentally related. A couple thoughts:

  • You mentioned concerns about breaking function purity when setting a default title, but if that logic were done at render time (e.g. in a selector or the component itself) I don't think there would be any purity problem? Definitely agree that setting a default title in the store is unappealing
  • I also don't mind a style fix in general, but maybe you could set a min-height on the element that contains the project title, in em?

@z1ndabad
Copy link
Collaborator Author

Thanks! Will play with both of those and revise.

@outoftime
Copy link
Contributor

Maybe an even cuter solution: Add an ::after pseudoselector that adds some arbitrary character (like a . ) with visibility hidden

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.

Untitled Project in Project Picker
2 participants