Refactor modal content (fixes #291 and @292) #324

OtisRed · 2020-11-11T21:01:37Z

Story / Bug id:

#291 and #292

Description:

It was easier for me conceptually to go through both those tickets at the same time. Most important changes incude:

Title breaks by words in case of overlay - it was breaking by letters before.
I introduced h2 and h3 tags to structure the content for SEO reasons.
In mobile view I moved the close button to the bottom-right corner. It made a lil bit more space for the title on smaller devices and made it easier to use close button in mobile view (it's closer to a thumb).
I centered all the content. Seems more intuitive this way.
All the buttons are rendered conditionally based on the condition that there actually is something to show.
"Partner" and "stack" sections are also rendered conditionally.
I recomposed partner/s section, because we had long text buttons that wouldn't break in case of an overlay.
I dropped v-btn, v-icon and v-card - I styled them with CSS and moved some scoped classes to global.scss

Migrations:

I nested partner partners' attributes in the partner object, which is a list now.

New imports / dependencies:

N/A

What tests do I need to run to validate this change:

Run in docker
Check visually - especially on smaller screens
Check if conditional rendering works properly

agatakozinska · 2020-12-09T20:36:54Z

frontend/src/components/ModalContent.vue

-        </v-card-actions>
-      </v-row>
+    <v-card-title class="container-title unified-padding">
+      <h1>{{ selectedProject.name }}</h1>


If it is possible to give it a class, that way we remain a flat hierarchy of selectors. Or if styles for headings can be used also in other components you can reconsider moving it to global styles

Given our discussion of h1 tags we should decide whether using h1 here is appropriate at all. Whatever the class should be, I wouldn't argue moving it, at least in part, to the global class. Will keep you posted on the adjustments.

Final changes:

I removed h1 tag to reserve it for singular use only - although multiple h1 tags don't matter anymore in html 5, because web crawlers use sections to understand the structure in more modular way (e.g. Google) people still believe that it is the best practice. Basically the argument is that with the singular use you don't lose anything but you may gain sth in particular cases.

So I used h2 and h3 tags instead - both went to global classes with some local override for the main title.

agatakozinska · 2020-12-09T23:02:46Z

frontend/src/components/ModalContent.vue

+      <h1>{{ selectedProject.name }}</h1>
+      <div>
+        <v-btn class="hidden-xs-only" rounded icon large @click="onClick()">
+          <v-icon size="3.5rem" color="white">close</v-icon>


would be better to pass it as some variables not hardcoded like that, if not pass color as hex (#ffffff) not white
you can try this approach:
https://stackoverflow.com/a/61378302

Ok. so the options in the link regard the situation when I actually want to change sth about default vuetify settings. Given that I realize how fucked up it is to have part of the styling through vuetify props and others through regular classes is there actually a reason to try to overwrite that? I mean veutfy is what it is, but isn't it overkill to write css just to override the default settings to the same effect? Please straight me out if I'm wrong.

Final changes:

I dropped v-btn and v-icon altogether (in this case vuetify does more harm than good)

I created global classes for two kinds of buttons which use globally defined colors

agatakozinska · 2020-12-09T23:12:28Z

frontend/src/components/ModalContent.vue

+    <v-card-actions class="buttons-list unified-padding">
+      <div v-show="selectedProject.licensePage !== ''">
+        <v-btn text rounded :href="selectedProject.licensePage" target="_blank">
+          <v-icon>far fa-copyright</v-icon>


why not to use https://vuetifyjs.com/en/features/icons/#using-custom-icons and create component with icons? this way you write it once and then use it in a whole project
also I'd add some accessibility here like aria-label

That seems awsome. I'll try to implement that.

Final changes:
Global store for icons seems to be a really good idea but when I started playing with it both through Vuetify and Font Awsome I couldn't find the way that actually save code. To the contrary, it did introduce another layer of abstraction which doesn't seem that necessary at this point. For now I settled with the basic local use of FA with <i> tag.

I still believe global store for icons is a good idea but I think it goes beyond the scope of this task to do it right. I will create a new issue for that.

agatakozinska · 2020-12-09T23:12:55Z

frontend/src/components/ModalContent.vue

+      </div>
+      <div v-show="selectedProject.githubLink !== ''">
+        <v-btn text rounded :href="selectedProject.githubLink" target="_blank">
+          <v-icon>fab fa-github</v-icon>


agatakozinska · 2020-12-09T23:15:54Z

frontend/src/components/ModalContent.vue

+    <v-card-text class="unified-padding">
+      <p class="text-center">{{ selectedProject.description }}</p>
+    </v-card-text>
+    <div v-show="countPartners >> 0">


is >> it ok? not just > ?

Final changes:
You're right. For some reason webstorm was correcting me on a singular use, but actually it seems to work fine. FYI VSC doesn't claim that kind of problem :D

agatakozinska · 2020-12-09T23:18:27Z

frontend/src/components/ModalContent.vue

+          <v-col class="partner-item" cols="12" sm="3">
+            <v-btn class="buttons-chips" text :href="partners.link" rounded>
+              <span class="buttons-text">Poznaj</span>
+              <v-icon>fas fa-hands-helping</v-icon>


agatakozinska · 2020-12-09T23:19:43Z

frontend/src/components/ModalContent.vue

+h2 {
+  font-family: $font-content;
+  font-size: 1.5rem;
+  color: black;


use variable

Well... we didn't have global black defined :D I'll work on that => promise (I realize that promise isn't usually what you return in a function => just an inside joke :P )

Final changes:
That's done

agatakozinska · 2020-12-09T23:23:17Z

frontend/src/components/ModalContent.vue

-.buttons {
-  margin: 1rem 0.5rem 1rem 0;
+.buttons-close {
+  background-color: $blue !important;


is !important needed? I guess you can add another class to the element and then you don't have to use such an overwriting

Yes, this is the kind of moment when not using important results in nothing. So either I can use in-line prop where I cannot use $var and need to repeat $blue var's hex or can I use important to overrride whatever vuetify understands. I didn't figure out how to pass global $blue into html part of code. That's why this blasphemy is here

Final changes:
I dropped v-btn altogether so that's not an issue no more

agatakozinska · 2020-12-09T23:25:45Z

frontend/src/components/ModalContent.vue

+  justify-content: center;
+  text-align: center;
+  @media only screen and (max-width: $phone) {
+    padding: 2% 10%;


it's not easy to predict how it will behave on different devices (resolutions), if you use rem I would stay with that, not mixed

Good point. However if it was you to consider some kind of understand what measure would you use? Is rem the best choice?

Final changes:
U used REMs both for desktop and mobile view

agatakozinska · 2020-12-09T23:28:41Z

frontend/src/components/ModalContent.vue

+}
+
+.unified-padding {
+  padding: 0.5rem 1.5rem !important;


same as above - add another class, !imporant should be the laaaaaaaaaaast thing you can think about xd

Really it was. I usually use !importnat when I feel helpless with vuetify :P However this change had been override and I will actually try to replace vuetify for clear vue in the whole component. We'll see if that's needed then.

Final changes:
I dropped using vuetify's v-card to style the container so it's not a problem anymore

OtisRed added 8 commits November 11, 2020 21:38

added fab close button for xs screen

7f7b0d9

breaking words in titles

3a35a19

conditional rendering of buttons

c6c7521

almost there

c2767c0

css cleanup

8d7fed7

button problem solved and mobile-first implementation

c5f0c62

replaced v-spacer

a6cc856

drobiazgi

b44ee69

OtisRed changed the title ~~[WIP] Adjust heading in modal content (fixes #291)~~ [WIP] Adjust heading in modal content (fixes #291 and @292) Nov 25, 2020

OtisRed changed the title ~~[WIP] Adjust heading in modal content (fixes #291 and @292)~~ Refactor modal content (fixes #291 and @292) Nov 25, 2020

OtisRed added 2 commits December 10, 2020 00:00

cleaned up stylesheet with BEM naming

7b1d538

some more cleaning up

dcb75e4

agatakozinska suggested changes Dec 9, 2020

View reviewed changes

OtisRed added 17 commits December 10, 2020 00:49

some more cleaning up pt2

435dc3d

temp

27dcc2c

working commit

2038f3a

working commit

627bf7a

review response

1184266

minor change 1

4b0aaad

minor change 2

9ab43a1

minor change 3

7518cc8

minor change 4

9fcda3a

minor change 5

c0f66d7

minor change 6

2b2fe39

Merge branch 'master' into fixing_modal

839f7ea

added variable

c8608dc

replaced % with REM for width

06471c2

minor changes 7

2cec698

conditional rendering for stack icons (to come)

e57b68c

final cut

6d41de9

revert vuetify plugin

552afb3

OtisRed mentioned this pull request Jan 6, 2021

Fixing modal recov (WIP) #342

Closed

Merge remote-tracking branch 'upstream/master' into fixing_modal

b254b79

stanislawK approved these changes Feb 10, 2021

View reviewed changes

OtisRed merged commit d593504 into CodeForPoznan:master Feb 10, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor modal content (fixes #291 and @292) #324

Refactor modal content (fixes #291 and @292) #324

OtisRed commented Nov 11, 2020 •

edited

Loading

agatakozinska Dec 9, 2020

OtisRed Dec 17, 2020 •

edited

Loading

OtisRed Dec 31, 2020 •

edited

Loading

agatakozinska Dec 9, 2020

OtisRed Dec 17, 2020

OtisRed Dec 31, 2020

agatakozinska Dec 9, 2020

OtisRed Dec 17, 2020

OtisRed Dec 31, 2020 •

edited

Loading

agatakozinska Dec 9, 2020

OtisRed Dec 31, 2020

agatakozinska Dec 9, 2020

OtisRed Dec 17, 2020 •

edited

Loading

agatakozinska Dec 9, 2020

OtisRed Dec 31, 2020

agatakozinska Dec 9, 2020

OtisRed Dec 17, 2020 •

edited

Loading

OtisRed Dec 31, 2020

agatakozinska Dec 9, 2020

OtisRed Dec 17, 2020 •

edited

Loading

OtisRed Dec 31, 2020

agatakozinska Dec 9, 2020

OtisRed Dec 17, 2020

OtisRed Dec 31, 2020

agatakozinska Dec 9, 2020

OtisRed Dec 17, 2020

OtisRed Dec 31, 2020

Refactor modal content (fixes #291 and @292) #324

Refactor modal content (fixes #291 and @292) #324

Conversation

OtisRed commented Nov 11, 2020 • edited Loading

Story / Bug id:

Description:

Migrations:

New imports / dependencies:

What tests do I need to run to validate this change:

Choose a reason for hiding this comment

OtisRed Dec 17, 2020 • edited Loading

Choose a reason for hiding this comment

OtisRed Dec 31, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

OtisRed Dec 31, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

OtisRed Dec 17, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

OtisRed Dec 17, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

OtisRed Dec 17, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

OtisRed commented Nov 11, 2020 •

edited

Loading

OtisRed Dec 17, 2020 •

edited

Loading

OtisRed Dec 31, 2020 •

edited

Loading

OtisRed Dec 31, 2020 •

edited

Loading

OtisRed Dec 17, 2020 •

edited

Loading

OtisRed Dec 17, 2020 •

edited

Loading

OtisRed Dec 17, 2020 •

edited

Loading