Skip to content
This repository has been archived by the owner on Jun 25, 2020. It is now read-only.

Refactoring: _pages to the theme minimal-ionio #106

Closed
1 task done
epidrome opened this issue Apr 22, 2020 · 14 comments · Fixed by ioniodi/minimal-ionio#30 or #135
Closed
1 task done

Refactoring: _pages to the theme minimal-ionio #106

epidrome opened this issue Apr 22, 2020 · 14 comments · Fixed by ioniodi/minimal-ionio#30 or #135
Assignees
Labels
easy feedback needed green light approved for PR help wanted Extra attention is needed

Comments

@epidrome
Copy link
Member

epidrome commented Apr 22, 2020

Δήλωση Θέματος

Απορίες σχετικά με το θέμα (αν υπάρχουν)

Υπενθυμίσεις

  • Έχω ελέγξει ότι το περιέχομενο των αλλαγών μου δεν το έχει δηλώσει κάποιος άλλος
@AsteriosP
Copy link
Collaborator

όπως ανέφερα και στο τελευταίο μου review πιστεύω ότι είναι μία σωστή πρακτική να μεταφέρουμε τον κώδικα liquid στο θέμα καί όχι εδώ

έχω ξεκινήσει να δουλεύω πάνω σε αυτό.

@dimpram
Copy link
Collaborator

dimpram commented Apr 23, 2020

έχω ξεκινήσει να δουλεύω πάνω σε αυτό.

Αυτό το issue μπορεί να κλείσει με αφορμή τα 2 PR (ένα εδώ και ένα στο minimal-ionio) του @provopoulos καθώς από ότι βλέπω μεταφέρει το μοναδικό include που υπάρχει εδώ, στο minimal-ionio.

@epidrome epidrome changed the title Move include files and _pages to the theme minimal-ionio Move _pages to the theme minimal-ionio Apr 23, 2020
@epidrome
Copy link
Member Author

@JimDragon
δες τα σχόλια μου στα 2 αιτήματα που λες

επιπλεον το θέμα αναφέρεται και στον φάκελο των σελίδων που έχει κώδικα και είναι εδώ

@dimpram
Copy link
Collaborator

dimpram commented Apr 23, 2020

@epidrome ναι τα είδα και γενικά συμφωνώ με τα σχόλια σας αλλά εμένα η γνώμη μου είναι πως η συγκεκριμένη περίπτωση μπορεί να αποτελέσει "εξαίρεση" καθώς το dynamic.html είναι ένα αρχείο το οποίο έχει άμεση σχέση με την δουλειά του @provopoulos και για τους λόγους που έχω και εδώ πιστεύω οτι διευκολύνει την κατάσταση.

επιπλεον το θέμα αναφέρεται και στον φάκελο των σελίδων που έχει κώδικα και είναι εδώ

Εννοείτε το pages collection?

@provopoulos
Copy link
Collaborator

Το αρχικό PR του @Spirosvw ήταν καλά δομημένο υπό την έννοια πως η υλοποίηση δεν ήταν hardcoded, θα μπορούσε δηλαδή εξ' αρχής το dynamic.html να ενσωματωθεί στο αποθετήριο του θέματος.
Υπήρξε ένα μικρό πρόβλημα το οποίο χάλαγε την σωστή διάταξη του Twitter στο index.html με αποτέλεσμα να μην εμφανιζόταν σωστά στην στήλη το οποίο και διορθώθηκε.
Εφόσον λοιπόν έπρεπε να κάνω προσθήκη την υλοποίησή μου στο αποθετήριο θέματος αλλά και να κάνω μικρές αλλαγές στον κώδικα του Σπύρου στο index.html, το θεώρησα εύλογο να μεταφέρω το dynamic.html στο αποθετήριο θέματος.

@epidrome epidrome added easy green light approved for PR and removed PR welcome labels Apr 24, 2020
@epidrome
Copy link
Member Author

το θέμα κατοχυρώνεται στον @AsteriosP αν θες άνοιξε αντίστοιχο θέμα και εκεί αφού θα πρέπει να γίνουν δύο συντονισμένα αιτήματα

@provopoulos πράγματι είναι λίγο γκρι η κατάσταση και το πρόβλημα δεν είναι τόσο οι επιλογές που κάνουν οι συντελεστές αλλά η αρχιτεκτονική σε δύο αποθετήρια

@JimDragon θεωρείς πως υπάρχει κάποιο πρόβλημα με την αναβάθμιση του θέματος και αυτά τα αρχεία; νομίζω πως είναι ανεξάρτητα.

@provopoulos
Copy link
Collaborator

@epidrome
Τόσο η υλοποίηση του Σπύρου, όσο και η δικιά μου δεν έχουν πρόβλημα αναβαθμισιμότητας καθώς είναι ξεχωριστά, ανεξάρτητα αρχεία.

@dimpram
Copy link
Collaborator

dimpram commented Apr 24, 2020

@JimDragon θεωρείς πως υπάρχει κάποιο πρόβλημα με την αναβάθμιση του θέματος και αυτά τα αρχεία; νομίζω πως είναι ανεξάρτητα.

Δεν θα υπάρχει γιατί θα κρατήσουμε το index.html πάνω στο οποίο πατάνε και τα 2 αρχεία (dynamic.html, twitter-module.html).

@AsteriosP
Copy link
Collaborator

Δοκίμασα να περάσω το αρχείο courses.md από τον φάκελο _pages:site-gr στον φάκελο _layouts:minimal-ionio αλλά μετά την αλλαγή δεν έτρεχε σωστά η αντίστοιχη σελίδα.
@JimDragon @provopoulos μήπως έχετε να προτείνεται κάποιο workaround?

@dimpram
Copy link
Collaborator

dimpram commented May 6, 2020

Αυτό σου συνέβη μόνο στο courses.md? To courses.md δεν είναι layout, ανήκει στο collection _pages γιατί αποτελεί μια σελίδα που χρησιμοποιεί κάποιο layout. Αν θες να το κάνεις να είναι μέσα στον φάκελο layouts υποψιάζομαι θα πρέπει να αλλάξεις αρκετό κώδικα.

Η δική μου πρόταση είναι να περάσεις όλο αυτό το collection στο θέμα(αν γίνεται).

Διαφορετικά θα πρέπει να μετατρέψεις το κάθε page σε layout. Αυτό μπορεί να γίνει κάπως έτσι:

  1. Φτίαχνεις το αντίστοιχο layout στον φάκελο _layouts
  2. Κάνεις copy paste τον κώδικα
  3. Αλλάζεις το layout: στην σελίδα έτσι ώστε να χρησιμοποιεί τo κατάλληλο layout
    Επίσης δες και με το config αν χρειαστεί κάποια αλλαγη στα defaults

Μπορεί να υπάρχει και άλλος τρόπος αλλά αυτοί είναι που σκέφτηκα τώρα.

@AsteriosP
Copy link
Collaborator

@JimDragon το έχω δοκιμάσει και με το posts.html, αλλά συμβαίνει το ίδιο πρόβλημα.

Αυτό που σου εμφανίζει στο αντίστοιχο link για τα posts είναι αυτό που φαίνεται παρακάτω
image

θα δοκιμάσω αυτά που μου πρότεινες

@epidrome
Copy link
Member Author

epidrome commented May 7, 2020

ενημέρωσα το 1ο ποστ με το σχετικό θέμα #71 και πρόσθεσα την @korinaal

όπως το καταλαβαίνω τώρα η λύση δεν είναι απλά μετακίνηση και θα πρέπει να δούμε χωριστά κάθε αρχείο,

π.χ.:

  1. τα courses, groups θα μπορούσαν να μείνουν εδώ, αλλά κάπου έξω από τις συλλογές, αφού δεν είναι συλλογές}

  2. το people.md θα μπορούσε να γίνει layout στο θέμα

κτλ

@epidrome epidrome changed the title Move _pages to the theme minimal-ionio Refactoring: _pages to the theme minimal-ionio May 7, 2020
@stale
Copy link

stale bot commented May 21, 2020

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@korinaal
Copy link
Collaborator

Bump!! Το κοιτάω εγώ!!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
easy feedback needed green light approved for PR help wanted Extra attention is needed
Projects
None yet
6 participants